GNOME Bugzilla – Bug 757684
Implement an high level transcoding API (similar to GstPlayer but for transcoding)
Last modified: 2018-11-03 13:42:29 UTC
We should offer high level APIs for common purposes, so I am adding one for transcoding. I also implemented 2 elements to make everything more reusable: * uritranscodebin: A equivalent to playbin, where the user just has to specify 1 source URI, 1 destination URI and the wanted encoding target * transcodebin: A bin that will plugin decodebin [! some filters] ! encodebin.
Created attachment 314989 [details] [review] transcodebin: Implement uritranscodebin and transcodebin uritranscodebin take one `src-uri` and on `dest-uri` and just transcoding the input stream following the format define by the `profile` property transcodebin simply links one decodebin with an encodebin
Created attachment 314990 [details] [review] libs: Add a new GstTranscoder high level transcoding API And add a small gst-transcoder tool.
Created attachment 315014 [details] [review] libs: Add a new GstTranscoder high level transcoding API And add a small gst-transcoder tool.
Created attachment 315762 [details] [review] transcodebin: Implement uritranscodebin and transcodebin uritranscodebin take one `src-uri` and on `dest-uri` and just transcoding the input stream following the format define by the `profile` property transcodebin simply links one decodebin with an encodebin
Created attachment 315763 [details] [review] libs: Add a new GstTranscoder high level transcoding API And add a small gst-transcoder tool.
Anyone want to review that? I will need it in Pitivi, I might have it separated from -bad for the beginning but please let me know what you think :)
As discussed on IRC, I pushed a repo to https://github.com/thiblahute/gst-transcoder where we can make that API mature and get some more testing before we decide to move it to -bad.
(In reply to Thibault Saunier from comment #7) > As discussed on IRC, I pushed a repo to > https://github.com/thiblahute/gst-transcoder where we can make that API > mature and get some more testing before we decide to move it to -bad. I have made quite some work on it recently and I have the impression it would make sense to evaluate again for inclusion in 1.12, anyone against it? Should we make sure to get a review or it is not 100% mandatory for -bad? This also implements two new elements: * uritranscodebin: Equivalent to uridecodebin but for transcoding * transcodebin: Equivalent to playbin but for transcoding This code has been tested quite a bit as it has been used by Pitivi for the last year or so.
It should definitely get an API review before merging, even for -bad :)
(In reply to Tim-Philipp Müller from comment #9) > It should definitely get an API review before merging, even for -bad :) OK good, who is willing to do it? :) Also, the code is now at: https://github.com/pitivi/gst-transcoder not on my personal repository anymore.
I uploaded the API documentation here: https://people.freedesktop.org/~tsaunier/documentation/gst-transcoder/
Took a quick look -- thanks, I think this is API we need. 1. We have gst_transcoder_run() and gst_transcoder_run_async(), and it seems to me like users of the API might desire more control. Would it make sense to either: a. Add a cancellable to the async API b. Rethink the API in terms of start/pause/cancel, and have it always be async I have a preference for the latter as it is a bit closer to how pipelines work, anyway, and it's trivial for application to build sync mechanisms on top of it if they need it. 2. If the async mechanism is what we go with, then the position update at whatever intervals can probably just go, and we can leave that to clients too 3. Shouldn't avoid re-encoding be on by default? 4. (Bikeshed) 'dest', where it occur in the API, should be 'destination' -- I think it's better to not shorten these unless it really blows up the name length 5. (Bikeshed) Why do you have gst-libs/gst/transcoding/transcoder? Do you expect other work to turn up under the transcoding directory?
Thanks Arun for the review! (In reply to Arun Raghavan from comment #12) > Took a quick look -- thanks, I think this is API we need. > > 1. We have gst_transcoder_run() and gst_transcoder_run_async(), and it seems > to me like users of the API might desire more control. Would it make sense > to either: > > a. Add a cancellable to the async API > > b. Rethink the API in terms of start/pause/cancel, and have it always be > async It is the API I have in mind but kept it very simple, will add .pause() and .cancel(). The run () API is really a very high level helper for user writing scripts etc so you can just do Gst.Transcoder(input, output, format).run(), the goal is to be very high level and simple to use thus this API, I have the feeling it is nice to have. > I have a preference for the latter as it is a bit closer to how pipelines > work, anyway, and it's trivial for application to build sync mechanisms on > top of it if they need it. Right, this is what we have/want indeed. > 2. If the async mechanism is what we go with, then the position update at > whatever intervals can probably just go, and we can leave that to clients too No, I want to have an API very similar to GstPlayer which is why I have that, and if you look at the rest of the API, it is very very similar. > 3. Shouldn't avoid re-encoding be on by default? Well, I did what encodebin does, not sure, we can bikeshed on IRC about that :) > 4. (Bikeshed) 'dest', where it occur in the API, should be 'destination' -- > I think it's better to not shorten these unless it really blows up the name > length Hrm, no idea, please others tell me what you prefer :) > 5. (Bikeshed) Why do you have gst-libs/gst/transcoding/transcoder? Do you > expect other work to turn up under the transcoding directory? Yes, the whole API has the goal of enabling a client/server distributed encoding/transcoding capabilities, and this is the first part which is basically the basis of a client.
All of this is fine, then. I'm not entirely sold on sync API or the position reporting mechanism, but maybe I'm wrong and we shouldn't force all our API users to have to think in async terms only. I guess the thing only other comment I have is that you'll need keep a consistent API with pause()/cancel() when used after either a sync or async run().
Could you update the patches please, or are they the latest version already?
(In reply to Arun Raghavan from comment #14) > All of this is fine, then. I'm not entirely sold on sync API or the position > reporting mechanism, but maybe I'm wrong and we shouldn't force all our API > users to have to think in async terms only. What is your actual concern about the position reporting mechanism? I am not sure I understand it. > I guess the thing only other comment I have is that you'll need keep a > consistent API with pause()/cancel() when used after either a sync or async > run(). Sure, that should be no problem. (In reply to Tim-Philipp Müller from comment #15) > Could you update the patches please, or are they the latest version already? Well, there are many more patches now and it is all in https://github.com/pitivi/gst-transcoder/commits/master - If we say go with that in -bad I will produce updated patches to merge it all in there, sure, but meanwhile I am still working on its own repository and would very much like to avoid adding more work than necessary for merging :)
I'd also rather target this for merging early in the 1.13 cycle tbh, rather than squeeze it in now.
(In reply to Tim-Philipp Müller from comment #17) > I'd also rather target this for merging early in the 1.13 cycle tbh, rather > than squeeze it in now. Right, I agree, let's do that then!
(In reply to Thibault Saunier from comment #16) > (In reply to Arun Raghavan from comment #14) > > All of this is fine, then. I'm not entirely sold on sync API or the position > > reporting mechanism, but maybe I'm wrong and we shouldn't force all our API > > users to have to think in async terms only. > > What is your actual concern about the position reporting mechanism? I am not > sure I understand it. Mostly about adding complexity to the API by adding a reporting mechanism, rather than the client implementing their own polling. I guess, it's the same as adding a sync and async API when async will do (and client can do sync themselves). I prefer a leaner API, and perhaps what you've done is more convenient from the API user's perspective.
(In reply to Tim-Philipp Müller from comment #17) > I'd also rather target this for merging early in the 1.13 cycle tbh, rather > than squeeze it in now. Time to get that done? :-)
What's left to do here? We had one bikeshed comment about 'dest' vs 'destination' in the API. The rest of what I had to say were just opinions/comments that are subjective enough to consider resolved.
I'd still like to have a look at the API as well, since I've done a similar API recently too. I'll try to do it this week.
(In reply to Tim-Philipp Müller from comment #22) > I'd still like to have a look at the API as well, since I've done a similar > API recently too. I'll try to do it this week. Did you mean this week? :-)
Let's try to get it in soon this cycle? :-)
Yes indeed, sorry for dropping the ball. As mentioned on irc, I did look at it, but this API doesn't feel quite right to me as-is. It doesn't do the things I would expect from such an API. I don't know yet if this is just a matter of some missing bits that can easily be added on top of what you have or if it should be changed a bit. I will try to come up with a list of requirements as I see them, then we can go over that. We should also see how the GstPlayer signal/messaging stuff will be refactored and update accordingly.
The .new() method should probably be failable or do you fail on .run() if the profile is invalid (or not deserializable?). I must say I quite like the simplicity of the current API. One extra feature I'd like is some API to add filters for simple operations, things like cropping, scaling, boxing, etc. Also some API to change start/end/duration of the result, for very basic editing. But I think those could be added to the current base. transcodebin also missed a subtitles filter (for compleness). Also not convinced about the whole dispatcher thing, feels very complicated..
(In reply to Olivier Crête from comment #26) > The .new() method should probably be failable or do you fail on .run() if > the profile is invalid (or not deserializable?). The _new() will return NULL, it is indeed not ideal and not binding friendly, should I make it a GInitable? Optionally I can make it fail in run() instead? > I must say I quite like the > simplicity of the current API. One extra feature I'd like is some API to add > filters for simple operations, things like cropping, scaling, boxing, etc. > Also some API to change start/end/duration of the result, for very basic > editing. But I think those could be added to the current base. There is already APIs for that using the `transcodebin::video_filter` and `transcodebin::audio_filter` properties (in Pitivi we use that to generate thumbnail cache while transcoding proxy assets), adding higher level APIs for that in `GstTranscoder` itself is definitely doable too. > transcodebin also missed a subtitles filter (for compleness). Indeed, that is not handled yet, nothing refraining us to add it. > Also not convinced about the whole dispatcher thing, feels very complicated.. My goal was to be exactly symmetric with the GstPlayer API (which is nice to use as end users FMPOV). Now, as stated by Tim, I should make sure to follow GstPlayer API changes this cycle.
(In reply to Thibault Saunier from comment #27) > > Also not convinced about the whole dispatcher thing, feels very complicated.. > > My goal was to be exactly symmetric with the GstPlayer API (which is nice to > use as end users FMPOV). Now, as stated by Tim, I should make sure to follow > GstPlayer API changes this cycle. I'm going to work on that at the GStreamer hackfest, so let's sit together there maybe?
> I'm going to work on that at the GStreamer hackfest, so let's sit together there maybe? Sounds like a plan! :-)
(In reply to Thibault Saunier from comment #27) > (In reply to Olivier Crête from comment #26) > > The .new() method should probably be failable or do you fail on .run() if > > the profile is invalid (or not deserializable?). > > The _new() will return NULL, it is indeed not ideal and not binding > friendly, should I make it a GInitable? Optionally I can make it fail in > run() instead? Yes, if _new can fail, it should be a GInitable instead and return a GError in the _new too I think? Would make bindings life easier.
> Yes, if _new can fail, it should be a GInitable instead and return a GError in the _new too I think? Would make bindings life easier. Erg, this brings a dep on GIO in, not sure we want that?
Well, if you want to add an asynchronous API, GIO way is superior to anything we have ever achieved in Gst. I've very often heard this non-argument, but can anyone provide real rational against it ?
GIO is huge unfortunately, on e.g. Android/iOS pulling it in considerably increases binary size for useless features like GDbus and all the desktop integration bits that really shouldn't be in GIO in the first place.
I've heard that story a lot of time, and still we ship libsoup that pulls it. What's the real number for this ? Do we ever run without HTTPs support ?
I also tend to think that in practice *very few* people will use GStreamer without GIO tbh. Though, to me, GInitable should not be in GIO!
(In reply to Nicolas Dufresne (stormer) from comment #34) > I've heard that story a lot of time, and still we ship libsoup that pulls > it. What's the real number for this ? Do we ever run without HTTPs support ? Not every Android application is using libsoup, and some people care about binary sizes :) I don't have the numbers for that currently, but considering the Olivier also provided patches to disable parts of GIO, he probably has some numbers to share.
So, if I read you correctly, we should avoid good APIs because the library split up in GLib GIO need to be fixed. That make no sense to me. If needed, we should provide patches to GLib to be able to remove the part that make no sense for an OS. If that means splitting GIO into multiple parts, then this is what we should do. I think it's time we stop avoiding good APIs for the reason that we don't have time to propose fixes in GLib. I'm pushing that way, because Philippe came to me asking why we don't provide more patches for these valid use cases (he was referring to Olivier's work).
OK, I handle the profile creation issue at the time of running now, I think it is event better APIwise as it allows handling the error as any other issue for the users.
(In reply to Nicolas Dufresne (stormer) from comment #37) > [...] asking why we don't provide more patches > for these valid use cases (he was referring to Olivier's work). I provided a proof-of-concept patch (I had a hackish patch that removed much more stuff back then, but that was the "clean" version of part of it for discussion) for that 4 years ago when I worked on a project that was quite space-constrained, and it wasn't exactly met with a lot of enthusiasm by GLib people. See bug #727798, and Matthias' reply "I'm not interested in this.". Similar comments were made on IRC back then. What that bug devolved into then was looking at having the linker fix the problem for us, but that was overall not very successful. Maybe the situation is different nowadays > If that means splitting GIO into multiple parts Splitting GIO would be ABI breakage, it's too late for fixing this properly now until GLib 3 happens, if it ever happens. So the best that can be done here is optionally disabling parts of GIO.
> Well, if you want to add an asynchronous API, GIO way is superior to anything we have ever achieved in Gst. That's certainly true and the async API in GIO is great if you can rely on the existence of a GMainLoop, which we unfortunately generally can't in GStreamer.
On Android/iOS the problem isn't really that those APIs are in libgio, the problem is that the implementation of GIO Extension points is stupid because they all get registered at the start instead of just when they are called the first time, so they get pulled in static builds. I think with a little work, I can come with a patch that fixes that in GLib so GDBus/etc don't get pulled into static builds. I think for async APIs, we should just say that a GMainContext/GMainLoop is a requirement, possibly we can just create a small helper that creates a GMainContext and runs it in a new thread until all of the async operations associated with it have completed for those we don't natively use a GMainContext instead of inventing yet another thing that makes the common use-case more painful.
Some backlog from our last conversation with Sebastian about that on IRC: > <thiblahute> What would you do about GstTranscoder? > <slomo> thiblahute: that depends what your realistic plans are and if someone is actually using it :) imho it would be useful to have, and then people can come with suggestions about how to improve the API > <thiblahute>slomo: Well I do not plan on doing a *big* rewamp of the API as we would like, but I am definitely willing to make changes to improve what I have now > <slomo> thiblahute: as long as it's not merge & abandon, i think it would be good to have merged into -bad > <thiblahute> Well, we **heavily** use it in Pitivi so it won't be abandonned > <slomo> ah i forgot about that > <slomo> i guess that's also good proof that it actually works ;) So my plan would be to merge if noone is strongly against doing so. The API won't be stable and gainning feedback from more people would be nice!
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/322.