GNOME Bugzilla – Bug 734902
vaapiencode_h264: outputs wrong caps, adds "codec_data" field for bytestream caps
Last modified: 2014-10-29 14:35:27 UTC
$ gst-launch-1.0 videotestsrc num-buffers=1 ! vaapiencode_h264 ! fakesink -v GstVaapiEncodeH264:vaapiencodeh264-0.GstPad:src: caps = video/x-h264, alignment=(string)au, stream-format=(string)byte-stream, ... , codec_data=(buffer)0142c00dffe100126742c00dab40a0f90800000008000001e42001000468ce3c80 This is not right. There should only be a codec_data field with stream-format=avc caps. For byte-stream format, you could optionally put the SPS/PPS into the caps in form of a streamheader field, but then in bytestream nal format, ie. with sync markers.
Basically the core libgstvaapi is intended to support byte-stream format only but of course it can provide the codec-data if needed. There are two ways to resolve this issue: 1: Don't attach any codec-data from gstvaapi plugin, and if format conversion needed use h264parse. Which means vaapiencode_h264 will only support byte-stream. 2: Implement separate routine for avc converion in core libgstvaapi and use it based on negotiated caps in gstvaapi plugin. Which means vaapiencode_h264 can support only both avc and byte-stream. Approach-1 should be enough in gstreamer point of view. Approach-2 might be good if we need to make libgstvaapi as a separate library.
Aha, sorry the avc-covert code is already there :)
Created attachment 286636 [details] [review] encode: Attach the codec-data to out caps only based on negotiated caps
@Gwenole: It could be good to move the byte_stream_to_avc() routine from gst/vaapi to libgstvaapi as a public API. WDT?
Review of attachment 286636 [details] [review]: OK with the mentioned changes. Thanks. ::: gst/vaapi/gstvaapiencode_h264.c @@ +240,3 @@ encode->is_avc ? "avc" : "byte-stream", NULL); + base_encode->need_codec_data = encode->is_avc ? TRUE : FALSE; base_encoder->need_codec_data = encoder->is_avc; is just fine. No need for the ternary expression. :)
Created attachment 289503 [details] [review] encode: Attach the codec-data to out caps only based on negotiated caps
Thanks for the review. I can push the patch if you have no other concerns. I will create a new bug report too, for tracking the stuff mentioned in comment4.
(In reply to comment #7) > Thanks for the review. > I can push the patch if you have no other concerns. > I will create a new bug report too, for tracking the stuff mentioned in > comment4. Yes, thanks, the patch was pre-approved with the mentioned change. :)
(In reply to comment #4) > @Gwenole: It could be good to move the byte_stream_to_avc() routine from > gst/vaapi to libgstvaapi as a public API. WDT? Yes, that should be fine to gstvaapiutils_h264. I would say to even opt for something like: gst_vaapi_utils_h264_convert_byte_stream_to_avc_buffer_ip(), where _ip = in-place and the params be guchar *buf, gsize buf_size. i.e. buffer_map()/unmaps happens on the call site. And optimization part would be to use the mpeg2 scan_for_startcodes() for h264 too. Though, that's be an optional/second patch. Thanks.
commit 7f3795c9fb8b974082cead5a18713df9e2060cba Author: Sreerenj Balachandran <sreerenj.balachandran@intel.com> Date: Wed Oct 29 15:46:12 2014 +0200 encode: Attach the codec-data to out caps only based on negotiated caps Attach the codec_data to out_caps only if downstream needed. For eg: h264 encoder doesn't need to stuff codec_data to the src caps if the negotiated caps has a stream format of byte-stream. https://bugzilla.gnome.org/show_bug.cgi?id=734902