GNOME Bugzilla – Bug 705452
h264parse: Does not extract width/height/etc from h264-in-mpeg-ps
Last modified: 2014-11-12 16:20:26 UTC
$ gst-launch-1.0 videotestsrc num-buffers=200 ! x264enc ! mpegpsmux ! filesink location=/tmp/h264.mpg $ G_DEBUG=fatal_warnings gst-launch-1.0 playbin uri=file:///tmp/h264.mpg Setting pipeline to PAUSED ... Pipeline is PREROLLING ... ** (gst-launch-1.0:2029): CRITICAL **: gst_video_decoder_negotiate_default: assertion `GST_VIDEO_INFO_WIDTH (&state->info) != 0' failed Trace/breakpoint trap Program received signal SIGTRAP, Trace/breakpoint trap.
+ Trace 232339
Thread 140737199171328 (LWP 2058)
The problem here seems to be that h264parse does not extract width/height or anything from the h264 stream.
Created attachment 252371 [details] [review] h264parse: do not set CAPS before SPS/PPS nals parsing
Created attachment 252373 [details] [review] h264parse: do not set CAPS before SPS/PPS nals parsing
Review of attachment 252371 [details] [review]: ::: gst/videoparsers/gsth264parse.c @@ +1888,3 @@ + /* do not set CAPS before SPS/PPS nals parsing */ + if (!h264parse->have_sps || !h264parse->have_pps) + return TRUE; I'm not sure if that is enough, also it would seem cleaner to just put the negotiate block below in an if (have_sps && have_pps) block. The problem I see here is that handle_frame() will immediately call negotiate() again, which might be before the SPS/PPS are parsed.
(In reply to comment #4) > Review of attachment 252371 [details] [review]: > > ::: gst/videoparsers/gsth264parse.c > @@ +1888,3 @@ > + /* do not set CAPS before SPS/PPS nals parsing */ > + if (!h264parse->have_sps || !h264parse->have_pps) > + return TRUE; > > I'm not sure if that is enough, also it would seem cleaner to just put the > negotiate block below in an if (have_sps && have_pps) block. Fixed locally. > > The problem I see here is that handle_frame() will immediately call negotiate() > again, which might be before the SPS/PPS are parsed. As far i understand calling _negociate without SPS/PPS is not a problem since it does not directly set the caps but rather update it + format/align. Only _update_src_caps should not be called before any SPS/PPS parsing. Calls to _update_src_caps are automatically triggered when SPS/PPS are parsed (h264parse->update_caps = TRUE). Should a check be added before calling _negociate in _handle_frame() ?
Ah good point. No that should be fine then :)
Created attachment 252438 [details] [review] h264parse: do not set passthrough mode before SPS/PPS nals parsing
(In reply to comment #6) > Ah good point. No that should be fine then :) The check could even be placed to not enable passtrough mode if SPS/PSS has not been parsed yet. This won't hijack the code path below if input != output and input is avc. The question now is, should the passtrough mode enabled later ?
Comment on attachment 252438 [details] [review] h264parse: do not set passthrough mode before SPS/PPS nals parsing I don't know why but this patch alone causes new caps to be set for every single buffer it seems, and also the initial caps don't have width/height set. Setting pipeline to PAUSED ... Pipeline is PREROLLING ... /GstPipeline:pipeline0/GstH264Parse:h264parse0.GstPad:src: caps = video/x-h264, parsed=(boolean)true, stream-format=(string)byte-stream, alignment=(string)au /GstPipeline:pipeline0/GstFakeSink:fakesink0.GstPad:sink: caps = video/x-h264, parsed=(boolean)true, stream-format=(string)byte-stream, alignment=(string)au /GstPipeline:pipeline0/GstH264Parse:h264parse0.GstPad:sink: caps = video/x-h264 /GstPipeline:pipeline0/GstH264Parse:h264parse0.GstPad:src: caps = video/x-h264, width=(int)320, height=(int)240, parsed=(boolean)true, stream-format=(string)byte-stream, alignment=(string)au, pixel-aspect-ratio=(fraction)1/1 /GstPipeline:pipeline0/GstFakeSink:fakesink0.GstPad:sink: caps = video/x-h264, width=(int)320, height=(int)240, parsed=(boolean)true, stream-format=(string)byte-stream, alignment=(string)au, pixel-aspect-ratio=(fraction)1/1 [...]
Your first patch was setting the width/height on the initial caps, but there was also new caps (the same) for every single buffer
(In reply to comment #10) > Your first patch was setting the width/height on the initial caps, but there > was also new caps (the same) for every single buffer This has something to do with enabling the passthrough mode at some point. I should be able to send patch by the end of the week.
Created attachment 253232 [details] [review] h264parse: do not set CAPS and passthrough mode if SPS/PPS have not been parsed
Created attachment 253233 [details] [review] h264parse: only update src CAPS when it's necessary
commit 4b10f278b6268b01d16052257d5da7c9ba985f03 Author: Matthieu Bouron <matthieu.bouron@collabora.com> Date: Tue Aug 27 11:27:04 2013 +0100 h264parse: only update src CAPS when it's necessary https://bugzilla.gnome.org/show_bug.cgi?id=705452 commit 43dcebe2a024547d04cd4d7fac41cc88c6d13dd6 Author: Matthieu Bouron <matthieu.bouron@collabora.com> Date: Tue Aug 20 11:59:34 2013 +0100 h264parse: do not set CAPS and passthrough mode if SPS/PPS have not been parsed https://bugzilla.gnome.org/show_bug.cgi?id=705452
*** Bug 708240 has been marked as a duplicate of this bug. ***