GNOME Bugzilla – Bug 692933
[API] codecparsers: mpeg2: add new API that takes GstMpegVideoPacket arguments
Last modified: 2013-07-08 09:18:06 UTC
Created attachment 234899 [details] [review] codecparsers: mpeg2: add new API that takes GstMpegVideoPacket arguments Hi, This patch adds new interface to MPEG-2 video parser so that functions take a GstMpegVideoPacket argument instead of data, size and offset. New functions are called after gst_mpeg_video_packet_*() and provide the default implementation. Older API is moved to the deprecated API and re-use the new functions. The new API is more convenient in practice, since we normally get a correct packet out of gst_mpeg_video_parse(). i.e. we just need to re-use it as is when each individual parser is called. This also makes it possible to add an API for slice() header which is more consistent/in-line with the rest of the mpeg2 codec parser.
I think this makes sense and is nicer API too. Not entirely sure about the naming and order of arguments though (the inevitable bikeshedding, apologies): if the API is called gst_blah_packet_parse_foo() then the first argument should probably be the packet. That way bindings can map it to packet.parse_foo(). Or you make it gst_blah_parse_foo_from_packet(), then packet is the last arg.
(In reply to comment #1) > Not entirely sure about the naming and order of arguments though (the > inevitable bikeshedding, apologies): if the API is called > gst_blah_packet_parse_foo() then the first argument should probably be the > packet. That way bindings can map it to packet.parse_foo(). Or you make it > gst_blah_parse_foo_from_packet(), then packet is the last arg. I am not sure about the naming of the functions either. :) (i) If we make it gst_mpeg_video_packet_parse_something(packet, hdr, ...), then there could be cases where 1st arg (packet) is an input arg, second arg (the parsed header) an output arg, and extra args as input. e.g. for the slice() parser in particular, which requires an additional seqhdr for example. (ii) If we make it gst_mpeg_video_parse_something_from_packet(), then this is also fine but the names become very long. I would be tempted by (i) + fixing up the args order, but I definitely have no clear preference. Opinions?
Created attachment 234913 [details] [review] codecparsers: mpeg2: add new API that takes GstMpegVideoPacket arguments Implemented solution (i) with GstVideoMpegPacket args swapped.
Looks good to me. Could you also update the mpegvideoparser in gst/videoparsers/ to the new API ?
Oh, and could you add * Since: 1.2 */ markers at the end of gtk-doc chunks for the new API you added?
Created attachment 248372 [details] [review] codecparsers: mpeg2: add new API that takes GstMpegVideoPacket arguments Here is the updated patch to match current git master branch. Diff of the patches shows the addition of "Since: 1.2" tags and minor fixes to function names for gtk-doc.
Created attachment 248373 [details] [review] mpegvideoparse: cope with new parser API Patch to update the mpegvideoparser with newer APIs. I also tried to optimize gst_mpegv_parse_process_config() along the way by determining the extension_start_code_identifier before calling into the extension packet parser.
Review of attachment 248373 [details] [review]: ::: gst/videoparsers/gstmpegvideoparse.c @@ +318,3 @@ mpvparse->config_flags |= FLAG_MPEG2; + if (packet.size > 1) { + packet.type = packet.data[packet.offset] >> 4; Note: I am abusing packet.type here. It should be GST_MPEG_VIDEO_PACKET_EXTENSION and the sub_type parsed separately, but packet.type is not used in the codec parser library. But anyway, I will probably just fix it to the correct way to be on the safe side.
Created attachment 248401 [details] [review] mpegvideoparse: cope with new parser API
You have to add them to docs/libs/gst-plugins-bad-libs-sections.txt :)
Looks fine to me, feel free to push after adding the new functions to the docs. Thanks for updating the patches. Regarding comment #8 - yes, if in doubt it's best to write code with readability and maintainability in mind IMHO, or to at least add a comment about the abuse.
The patches have committed. Please close the bug.
commit daddc1d7d61e9edd918ca3b4f404a655459fce44 Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com> Date: Thu Jan 31 11:57:54 2013 +0100 codecparsers: mpeg2: update test program with new GstMpegVideoPacket API. This is the lost hunk from: https://bugzilla.gnome.org/show_bug.cgi?id=692933 Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com> commit d10acd38790cd0f3d7b1318ff694281d67ddf473 Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com> Date: Thu Jul 4 14:22:14 2013 +0200 mpegvideoparse: cope with new parser API. Migrate the code to use the new parser API based on GstMpegVideoPacket. Also try to optimize gst_mpegv_parse_process_config() by using more of GstMpegVideoPacket and determining the extension_start_code_identifier prior to calling the parser function for that extension packet. Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com> commit 1a5265ef72375fc1cf383b9e707a998e7c293887 Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com> Date: Thu Jan 31 11:57:54 2013 +0100 codecparsers: mpeg2: add new API that takes GstMpegVideoPacket arguments. Add new interface to MPEG-2 video parser that takes GstMpegVideoPacket arguments instead of data, size, and offset. New functions are called after gst_mpeg_video_packet_*() and provide the default implementation. Older API is moved to the deprecated namespace and uses the new functions. https://bugzilla.gnome.org/show_bug.cgi?id=692933 Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com>