GNOME Bugzilla – Bug 622276
Add an h263parse element
Last modified: 2011-01-28 11:56:02 UTC
Created attachment 164206 [details] [review] h263parse: Add h263parse element I have written an h263parse element which determines stream codec and profile, and breaks up the incoming stream into frame-sized buffers. Attaching this as one large patch. The following repo might make for easier reviewing, however: http://git.collabora.co.uk/?p=user/arun/gst-plugins-bad.git;a=summary
Review of attachment 164206 [details] [review]: ::: gst/h263parse/h263parse.c @@ +376,3 @@ +gst_h263_parse_get_params (GstH263Parse * h263parse, H263Params * params, + gboolean fast) +{ it would be a bit cleaner if this was in a separate file ? Along with all the parsing-specific structures. Then it would leave only the gst-specific parts in gsth263parse.[ch] @@ +808,3 @@ + +static gint +gst_h263_parse_get_profile (GstH263Parse * h263parse) Same thing, this could be moved to a separate file and just take a H263Params structure as input. @@ +930,3 @@ + +static gint +gst_h263_parse_get_level (GstH263Parse * h263parse) Same comment as for get_profile @@ +1081,3 @@ + if (h263parse->profile != -1) + gst_caps_set_simple (caps, "profile", G_TYPE_STRING, + profiles[h263parse->profile], NULL); If profile is only going to be a numerical value, let's keep it as an integer. @@ +1086,3 @@ + if (h263parse->level != -1) { + level_idx = (h263parse->level / 10) + (h263parse->level % 10) - 1; + gst_caps_set_simple (caps, "level", G_TYPE_STRING, levels[level_idx], Same comment as for profile. ::: gst/h263parse/h263parse.h @@ +67,3 @@ + H263_OPTION_DPS_MODE = 1 << 14 +} H263OptionalFeatures; + Since this is a plugin, there's maybe no need to have this flag definition here @@ +104,3 @@ + UUI_IS_01, +} H263UUI; + Same comment for all structures above. @@ +128,3 @@ + H263ParseState state; +} H263Params; + These too
(In reply to comment #1) > Review of attachment 164206 [details] [review]: > > ::: gst/h263parse/h263parse.c > @@ +376,3 @@ > +gst_h263_parse_get_params (GstH263Parse * h263parse, H263Params * params, > + gboolean fast) > +{ > > it would be a bit cleaner if this was in a separate file ? Along with all the > parsing-specific structures. Then it would leave only the gst-specific parts in > gsth263parse.[ch] Can do. > @@ +808,3 @@ > + > +static gint > +gst_h263_parse_get_profile (GstH263Parse * h263parse) > > Same thing, this could be moved to a separate file and just take a H263Params > structure as input. > > @@ +930,3 @@ > + > +static gint > +gst_h263_parse_get_level (GstH263Parse * h263parse) > > Same comment as for get_profile Can move to a separate file. get_level needs to take a GstH263Parse* since the bitrate isn't part of params (and IMHO should not be, since it basically represents stream params parsed out of the header). I let get_profile also take the same parameter for consistency, but I'm open to changing this. > @@ +1081,3 @@ > + if (h263parse->profile != -1) > + gst_caps_set_simple (caps, "profile", G_TYPE_STRING, > + profiles[h263parse->profile], NULL); > > If profile is only going to be a numerical value, let's keep it as an integer. > > @@ +1086,3 @@ > + if (h263parse->level != -1) { > + level_idx = (h263parse->level / 10) + (h263parse->level % 10) - 1; > + gst_caps_set_simple (caps, "level", G_TYPE_STRING, levels[level_idx], > > Same comment as for profile. Since some codecs have non-numeric profiles/levels, I figured we should just keep them all as strings instead of having to worry about which codec has a string profile vs. which has an integer profile. Again, I'm not too religious about this, so if the common feeling is to go with integers here, I'll change it. > ::: gst/h263parse/h263parse.h > @@ +67,3 @@ > + H263_OPTION_DPS_MODE = 1 << 14 > +} H263OptionalFeatures; > + > > Since this is a plugin, there's maybe no need to have this flag definition here > > @@ +104,3 @@ > + UUI_IS_01, > +} H263UUI; > + > > Same comment for all structures above. > > @@ +128,3 @@ > + H263ParseState state; > +} H263Params; > + > > These too Since the header is never installed, will this make a difference?
(In reply to comment #2) > > @@ +930,3 @@ > > + > > +static gint > > +gst_h263_parse_get_level (GstH263Parse * h263parse) > > > > Same comment as for get_profile > > Can move to a separate file. get_level needs to take a GstH263Parse* since the > bitrate isn't part of params (and IMHO should not be, since it basically > represents stream params parsed out of the header). I let get_profile also take > the same parameter for consistency, but I'm open to changing this. If you only need one extra parameter, can't you just add it as a return argument ? > > > > @@ +1081,3 @@ > > + if (h263parse->profile != -1) > > + gst_caps_set_simple (caps, "profile", G_TYPE_STRING, > > + profiles[h263parse->profile], NULL); > > > > If profile is only going to be a numerical value, let's keep it as an integer. > > > > @@ +1086,3 @@ > > + if (h263parse->level != -1) { > > + level_idx = (h263parse->level / 10) + (h263parse->level % 10) - 1; > > + gst_caps_set_simple (caps, "level", G_TYPE_STRING, levels[level_idx], > > > > Same comment as for profile. > > Since some codecs have non-numeric profiles/levels, I figured we should just > keep them all as strings instead of having to worry about which codec has a > string profile vs. which has an integer profile. Again, I'm not too religious > about this, so if the common feeling is to go with integers here, I'll change > it. If for this media type we only have numerical values, let's stick with integers, makes the parsing code easier to handle (and therefore less bug-prone). > > > > ::: gst/h263parse/h263parse.h > > @@ +67,3 @@ > > + H263_OPTION_DPS_MODE = 1 << 14 > > +} H263OptionalFeatures; > > + > > > > Since this is a plugin, there's maybe no need to have this flag definition here > > > > @@ +104,3 @@ > > + UUI_IS_01, > > +} H263UUI; > > + > > > > Same comment for all structures above. > > > > @@ +128,3 @@ > > + H263ParseState state; > > +} H263Params; > > + > > > > These too > > Since the header is never installed, will this make a difference? Just cleaner code :)
Created attachment 165411 [details] [review] h263parse: Add h263parse element (updated) Incorporated all feedback and pushed to the tree I linked to previously. Attaching an updated, squashed patch.
Comment on attachment 165411 [details] [review] h263parse: Add h263parse element (updated) Looks good for committing once -bad is released.
Comment on attachment 165411 [details] [review] h263parse: Add h263parse element (updated) There is a bug in the current patch which breaks debug output. I'm going to try to move this to use baseparse, and will push an updated patch when I'm done.
Any news on this ?
I'm changing the code to use baseparse. Will post that here soon.
Created attachment 170829 [details] [review] baseparse: Fix debug output We lose the reference to the buffer after gst_pad_push(), so the debug print should happen before.
Created attachment 170830 [details] [review] baseparse: Allow chaining of subclass event handlers This allows the child class to chain its event handler with GstBaseParse, so that subclasses don't have to duplicate all the default event handling logic.
Created attachment 170831 [details] [review] h263parse: Add an h263parse element This adds an h263parse element for parsing H.263 streams, breaking them up into frame-sized buffers, and exporting metadata such as profile and level.
Attached a minor baseparse fix, one enhancement to allow event handler chaining, and subsequently a reworked h263parse element that derives from baseparse. For now, I've made a copy of gstbaseparse.[ch] in h263parse/, but this can be gotten rid of when baseparse moves to -base.
Created attachment 174049 [details] [review] jpegformat: Push tags after setting srcpad caps This patch defers emission of tag events till caps are set on the source pad of jpegparse, so that these tags can be seen downstream. https://bugzilla.gnome.org/show_bug.cgi?id=627211
Comment on attachment 174049 [details] [review] jpegformat: Push tags after setting srcpad caps Whoops, wrong patch. Sorry about the spam.
Comment on attachment 170831 [details] [review] h263parse: Add an h263parse element Attaching an updated patch with Mark's latest baseparse changes.
Created attachment 174050 [details] [review] h263parse: Add an h263parse element This adds an h263parse element for parsing H.263 streams, breaking them up into frame-sized buffers, and exporting metadata such as profile and level.
Created attachment 174089 [details] [review] h263parse: handle absent sink pad caps and improve baseparse keyframe handling Attached incremental patch (though it can be merged) should prevent critical complaints when using h263parse with raw input (no sink caps), and makes baseparse more keyframe aware (in particular not dropping keyframe before a segment).
Created attachment 174612 [details] [review] h263parse: handle absent sink pad caps and improve baseparse keyframe handling Despite testing, previous version of patch had a silly error, this updated one should now take care ...
So I now have h263parse pending locally, along with a (new) h264parse based on baseparse (though functionally equivalent to old h264parse). To limit proliferation of baseparse, the intended plan is: * have h263parse and h264parse in a videoparsers directory (along with a copy *ouch* of baseparse) * rename old h264parse to legacyh264parse (none rank and all) * remove legacyh264parse altogether after some grace period Note that baseparse itself has also seen (quite) some changes/updates, and will far less likely see more of those as long as several copies are out there, so it would be convenient (to say the least) if it could make it elsewhere (e.g. -base), albeit in whatever experimental state (worked long enough for playbin2). Comments ? Expected doom ?
> Comments ? Expected doom ? 1. w00t Moving forward I'd go for this: 2. let's add a -bad/gst/videoparsers/ with an internal copy of the modified baseparse for now, to get the code out there and get it tested etc. 3. let's keep the baseparses in gst/audioparsers/ and gst/videoparsers/ in sync 4. do some performance measurements to make sure we're not regressing majorly from the non-baseparse implementation (in h264parse) case, and also test backwards playback. 5. get API reviewed, also compare with GstBaseVideoParse in -bad/gst-libs/gst/video/ 6. then add new base class to core/base as appropriate (depends on whether we keep it generic or decide to go for dedicated audio/video classes I guess), and move code in -bad to that
commit b33b88e4bb2d192f7064feb558e325813f4b06ba Author: Arun Raghavan <arun.raghavan@collabora.co.uk> Date: Fri May 14 02:08:03 2010 +0530 h263parse: Add an h263parse element This adds an h263parse element for parsing H.263 streams, breaking them up into frame-sized buffers, and exporting metadata such as profile and level. https://bugzilla.gnome.org/show_bug.cgi?id=622276 (and some following ones ...)