GNOME Bugzilla – Bug 706211
applemedia: Garbled output from vtenc_h264
Last modified: 2013-11-29 08:59:52 UTC
I'm building gstreamer, and plugins-bad on macports using custom port files that retrieve the 1.1.3 developer release and build it. Launching the following pipeline fails $ gst-launch-1.0 --gst-debug-level=3 videotestsrc num-buffers=1000 ! videoconvert ! vtenc_h264 ! qtmux ! filesink location=test_new.mp4 Setting pipeline to PAUSED ... Pipeline is PREROLLING ... 0:00:00.033327000 57005 0x7fda4a806b20 FIXME default gstutils.c:3623:gchar *gst_pad_create_stream_id_printf_valist(GstPad *, GstElement *, const gchar *, __va_list_tag *):<videotestsrc0:src> Creating random stream-id, consider implementing a deterministic way of creating a stream-id 0:00:00.048729000 57005 0x7fda4a806b20 WARN default gstvideopool.c:171:video_buffer_pool_set_config:<videobufferpool0> no caps in config (gst-launch-1.0:57005): GStreamer-WARNING **: gstpad.c:4502:GstFlowReturn store_sticky_event(GstPad *, GstEvent *):<vtenc_h264-0:src> Sticky event misordering, got 'segment' before 'caps' 0:00:00.060236000 57005 0x7fda4a806b20 WARN basesrc gstbasesrc.c:2826:void gst_base_src_loop(GstPad *):<videotestsrc0> error: Internal data flow error. 0:00:00.060294000 57005 0x7fda4a806b20 WARN basesrc gstbasesrc.c:2826:void gst_base_src_loop(GstPad *):<videotestsrc0> error: streaming task paused, reason not-negotiated (-4) 0:00:00.060701000 57005 0x7fda4a806b20 FIXME basesink gstbasesink.c:3022:gboolean gst_base_sink_default_event(GstBaseSink *, GstEvent *):<filesink0> stream-start event without group-id. Consider implementing group-id handling in the upstream elements ERROR: from element /GstPipeline:pipeline0/GstVideoTestSrc:videotestsrc0: Internal data flow error. Additional debug info: gstbasesrc.c(2826): void gst_base_src_loop(GstPad *) (): /GstPipeline:pipeline0/GstVideoTestSrc:videotestsrc0: streaming task paused, reason not-negotiated (-4) ERROR: pipeline doesn't want to preroll. Setting pipeline to NULL ... Freeing pipeline ... I can however pipe a non-null output of vtenc_264 to a filesink for example.
For comparison, a pipeline like this, using x264enc works fine on the same system: $ gst-launch-1.0 --gst-debug-level=3 videotestsrc num-buffers=1000 ! x264enc ! qtmux ! filesink location=test.mp4
Created attachment 252057 [details] Screenshot of incorrect video output
The screenshot I attached is resulting from this pipeline: $ gst-launch-1.0 --gst-debug-level=3 videotestsrc num-buffers=1000 ! vtenc_h264 ! decodebin ! videoconvert ! autovideosink So, it looks like the encoder is producing something, but not quite right.
The warnings should be fixed of course, but I don't think they're related to any of the problems you're encountering, they're mostly harmless. I suspect the reason vtenc_h264 ! qtmux doesn't work is because vtenc_h264 outputs H.264 in byte-stream format, but qtmux wants it in raw AVC format. h264parse should be able to convert between the two formats. As for why the video in the screenshot is garbled: not sure, I would guess a bug in the encoder element.
Tim, thanks for the quick analysis. h264parse helps indeed, $ gst-launch-1.0 --gst-debug-level=3 videotestsrc num-buffers=1000 ! video/x-raw,format=NV12,width=800,height=600,framerate=\(fraction\)30/1 ! vtenc_h264 ! h264parse ! qtmux ! filesink location=test.mp4 works, but in fact does still produce the garbled output.
Created attachment 256231 [details] [review] Fixing gst to VT memory mapping by using GstVideoInfo and CVPixelBufferCreateWithPlanarBytes (2) Using CVPixelBufferCreateWithPlanarBytes allows better control for buffer handover to VT. By using a GstVideoInfo based mapping, we can extract all required video info details that are needed as arguments to the CoreVideo function. This fixes an incorrect interpretation of the input buffer which lead to the garbled output or crashes.
Review of attachment 256231 [details] [review]: Looks great! Just one nitpick from me. For consistency: + + + for (unsigned i = 0; i < num_planes; ++i) { Should be: + + for (size_t i = 0; i != num_planes; i++) {
Comment on attachment 256231 [details] [review] Fixing gst to VT memory mapping by using GstVideoInfo and CVPixelBufferCreateWithPlanarBytes (2) Updated after review: * unsigned -> size_t * replaced N_PLANES macro usage with local variable
Created attachment 256237 [details] [review] Fixing gst to VT memory mapping by using GstVideoInfo and CVPixelBufferCreateWithPlanarBytes (2) (never mind the last comment) Updated after review: * unsigned -> size_t * replaced N_PLANES macro usage with local variable
Thanks for the review, Ole!
Review of attachment 256237 [details] [review]: I'm afraid you missed two of my nitpicks (sorry I wasn't clear!), look at the subtle differences regarding "!=" and "++". :)
:-) Oups. Will update.
Created attachment 256240 [details] [review] Fixing gst to VT memory mapping by using GstVideoInfo and CVPixelBufferCreateWithPlanarBytes (3) Now hopefully all nits addressed.
Review of attachment 256240 [details] [review]: ::: sys/applemedia/vtenc.c @@ +781,3 @@ CVReturn cv_ret; + frame = gst_vtenc_frame_new (buf, &self->video_info); Should check for NULL here, you made it possible for the function to fail @@ +790,3 @@ + size_t plane_bytes_per_row[num_planes]; + + for (size_t i = 0; i != num_planes; i++) { Why != instead of < ? @@ +797,3 @@ + plane_bytes_per_row[i] = + GST_VIDEO_FRAME_COMP_STRIDE (&frame->videoframe, i); + plane_bytes_per_row[1] = should be [i], not [1] I guess
Sebastian: The != instead of < is my bad from when I wrote the first version of the plugin, I just thought it would be good to keep consistency and do a "sed"-powered swoop patch to address it for the applemedia plugin as a whole. :)
Review of attachment 256240 [details] [review]: ::: sys/applemedia/vtenc.c @@ +790,3 @@ + size_t plane_bytes_per_row[num_planes]; + + for (size_t i = 0; i != num_planes; i++) { Also as Tim noted on IRC, don't declare loop variables inside the loop. That's not C89 compatible :)
Created attachment 256250 [details] [review] Fixing gst to VT memory mapping by using GstVideoInfo and CVPixelBufferCreateWithPlanarBytes (4) Thanks for the review, comments addressed.
commit 24c79af39440e8b8642bc5e21ae463049dd09b1b Author: Dominik Röttsches <dominik.rottsches@intel.com> Date: Wed Oct 2 05:49:34 2013 +0300 vtenc: Use correct strides, etc from the GstVideoFrame https://bugzilla.gnome.org/show_bug.cgi?id=706211
Could you tell me why this does not seem to have been released in 1.2.1? Thanks.
Because I missed it for 1.2.1, it will be in 1.2.2
Alright - thanks! Can you merge the I420 support from bug 709241 for 1.2.2 as well?