GNOME Bugzilla – Bug 790000
rtph264depay: doesn't update resolution if format is avc
Last modified: 2018-02-01 10:23:58 UTC
Created attachment 363126 [details] [review] rtph264depay: pass sprop-parameter-sets for avc format A pipeline is failed if a resolution is changed while streaming avc format. This might be a kind of regression by this commit. https://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/gst/rtp/gstrtph264depay.c?id=e7e7f262009852a8515fa6db336138b61a4c3aef Reproducible steps 1. creating a series of images with different resolutions ``` $ gst-launch-1.0 videotestsrc num-buffers=10 ! video/x-raw,width=100,height=100 ! jpegenc ! multifilesink location=%03d.jpg $ gst-launch-1.0 launch videotestsrc num-buffers=10 ! video/x-raw,width=200,height=200 ! jpegenc ! multifilesink index=10 location=%03d.jpg ``` 2. then playing images ``` $ gst-launch-1.0 multifilesrc location=%03d.jpg ! jpegdec ! x264enc speed-preset=ultrafast ! rtph264pay ! rtph264depay ! h264parse config-interval=0 ! avdec_h264 ! fakesink ``` Error log ``` 0:00:00.066541310 14731 0x56481c5d7c00 WARN libav gstavviddec.c:1747:gst_ffmpegviddec_frame:<avdec_h264-0> avdec_h264: decoding error (len: -22, have_data: 0) 0:00:00.066577642 14731 0x56481c5d7c00 WARN videodecoder gstvideodecoder.c:1160:gst_video_decoder_sink_event_default:<avdec_h264-0> error: No valid frames decoded before end of stream 0:00:00.066588401 14731 0x56481c5d7c00 WARN videodecoder gstvideodecoder.c:1160:gst_video_decoder_sink_event_default:<avdec_h264-0> error: no valid frames found ERROR: from element /GstPipeline:pipeline0/avdec_h264:avdec_h264-0: No valid frames decoded before end of stream Additional debug info: ../subprojects/gst-plugins-base/gst-libs/gst/video/gstvideodecoder.c(1160): gst_video_decoder_sink_event_default (): /GstPipeline:pipeline0/avdec_h264:avdec_h264-0: no valid frames found ERROR: pipeline doesn't want to preroll. Setting pipeline to NULL ... Freeing pipeline ... ``` Before the commit, the pipeline plays well because rtph264depay adds SPS regardless formats. However, after the commit, 'avc' format doesn't have SPS so the pipeline can't know which properties are changed.
Last time I checked, rtph264depay still updates its output caps when the resolution changes. If so, this is correct behaviour and the problem is elsewhere. SPS/PPS should never be inline in the data with AVC (=avc1).
Created attachment 363131 [details] [review] rtph264depay: update codec_data Thanks for link the similar issue. This is caused because 'codec_data' is replaced with old one.
My patch is probably an opposing opinion about the previous code. According to the comment, it doesn't change caps if only 'codec_data' is changed, and h264 does like that. However, IMHO, the behaviours of these two elements need to be different. In case of h264parse, it can parse every useful information from caps of sinkpad, but not to send caps for srcpad if only codec_data is changed. That means h264parse can always use SPS/PPS from fresh codec_data. Whereas, if rtph264depay ignores codec_data like h264parse, there's no way to send new SPS/PPS for avc format because the information is in codec_data. Is there any nicer way to send SPS/PPS for avc format case?
(In reply to Justin J. Kim from comment #3) > My patch is probably an opposing opinion about the previous code. According > to the comment, it doesn't change caps if only 'codec_data' is changed, and > h264 does like that. I meant "the comment" in rtph264depay.c, not here. https://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/gst/rtp/gstrtph264depay.c?id=e7e7f262009852a8515fa6db336138b61a4c3aef#n404
Another possible solution I think is that 'rtph264depay' parses sprops-parameters-sets then sets width/height in src caps if the stream type is 'avc'. If this is better, codecutils should have h264spsparser which -bad currently has.
Created attachment 363321 [details] [review] rtph264depay: update srccaps regardless format `codec_data` should be transferred if any information of SPS/PPS is changed.
I printed log of old_s(gstrtph264depay.c:419) and tmp_s(gstrtph264depay.c:420) to see what happens while playing the above pipeline. https://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/gst/rtp/gstrtph264depay.c?id=e7e7f262009852a8515fa6db336138b61a4c3aef#n419 ``` 0:00:00.062779228 20366 0x564462944000 DEBUG rtph264depay gstrtph264depay.c:422:gst_rtp_h264_set_src_caps:<rtph264depay0> old_s video/x-h264, stream-format=(string)avc, alignment=(string)au, codec_data=(buffer)0142c015ffe1001b6742c015da1cfe79f016e04040b4a0000003002ee6b28001e2c5d401000468ce3c80, level=(string)2.1, profile=(string)constrained-baseline; 0:00:00.062789838 20366 0x564462944000 DEBUG rtph264depay gstrtph264depay.c:423:gst_rtp_h264_set_src_caps:<rtph264depay0> tmp_s video/x-h264, stream-format=(string)avc, alignment=(string)au, codec_data=(buffer)0142c015ffe1001c6742c015da0d1be597016e04040b4a0000030002ee6b28001e2c5d4001000468ce3c80, level=(string)2.1, profile=(string)constrained-baseline; ``` The other properties except for codec_data are same so if codec_data is ignored by gstrtph264depay.c:421, caps will not be updated. https://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/gst/rtp/gstrtph264depay.c?id=e7e7f262009852a8515fa6db336138b61a4c3aef#n421 This causes the situation that `h264parse` can't recognize changes. If sending codec_data, the case of #782949 seems to work, too.
ping?
Marking as blocker to make sure it gets looked at before the release (but don't have time to look right now, sorry).
I think this patch is correct, and slomo's change in 8dda570e4 (which introduced this behaviour) was just wrong. If the SPS/PPS change, h264parse should have the same behaviour. I know this will be a problem for mp4 file, in that case, h264parse should probably renumber the SPS if the goal is to create a single valid stream.. And it means that mp4mux would need to replace the codec_data it already wrote with the new one. Anyway, this isn't so relevant here.
(In reply to Olivier Crête from comment #10) > I think this patch is correct, and slomo's change in 8dda570e4 (which > introduced this behaviour) was just wrong. > > If the SPS/PPS change, h264parse should have the same behaviour. I only made it behave the same as h264parse back then, where it was intentionally done like this IIRC. Also that seems unrelated to this bug, or not? If the resolution changes, both would still update the caps. However I agree that a codec data change should be reflected in the (AVC) caps, always. Before doing that we will need to fix other elements (mp4mux specifically) though, as they will otherwise fail in situations that worked just fine before (which is why the commit in rtph264depay happened). > I know this > will be a problem for mp4 file, in that case, h264parse should probably > renumber the SPS if the goal is to create a single valid stream.. And it > means that mp4mux would need to replace the codec_data it already wrote with > the new one. Anyway, this isn't so relevant here. See above, it actually is quite relevant. We can't just change h264parse/rtph264depay and break situations that worked fine before, even if by accident. People rely on them :) Fixing mp4mux is going to be a little bit tricky though. Renumbering would have to happen like you say, but h264parse would have to detect this case (and you probably don't want to do this forever and keep around all SPS/PPS ever seen). And mp4mux would also have to handle this, which depending on the muxing mode might not be trivial.
(In reply to Sebastian Dröge (slomo) from comment #11) > > I know this > > will be a problem for mp4 file, in that case, h264parse should probably > > renumber the SPS if the goal is to create a single valid stream.. And it > > means that mp4mux would need to replace the codec_data it already wrote with > > the new one. Anyway, this isn't so relevant here. > > See above, it actually is quite relevant. We can't just change > h264parse/rtph264depay and break situations that worked fine before, even if > by accident. People rely on them :) I think it's fine to say that you need a h264parse before a mp4mux always and push this, as we're trying to fix a nastier regression where the SPS change is just not communicated. mp4mux can require parsed=true and so require a parser. > Fixing mp4mux is going to be a little bit tricky though. Renumbering would > have to happen like you say, but h264parse would have to detect this case > (and you probably don't want to do this forever and keep around all SPS/PPS > ever seen). And mp4mux would also have to handle this, which depending on > the muxing mode might not be trivial. Afaik, in the AVC1 case, it should renumber and keep them all around until the parser/depayloader is reset to READY as this is the only way to make a truly valid stream. As I think having "filesrc ! tsdemux! h264parse ! mp4mux ! filesink" should produce a single continuous file even if you do flushing seeks.
Created attachment 364616 [details] [review] rtph264depay: update output caps regardless format
I am not sure which problem mp4mux has. Here are my testing steps. It will be partly repetition of my first comment. Generated two different resolution images. $ gst-launch-1.0 videotestsrc num-buffers=10 ! video/x-raw,width=100,height=100 ! jpegenc ! multifilesink location=%03d.jpg $ gst-launch-1.0 videotestsrc num-buffers=10 ! video/x-raw,width=200,height=200 ! jpegenc ! multifilesink index=10 location=%03d.jpg Then, trying to mux them into a file. $ gst-launch-1.0 multifilesrc location=%03d.jpg caps="image/jpeg,framerate=(fraction)12/1" ! jpegdec ! x264enc ! rtph264pay ! rtph264depay ! h264parse config-interval=0 ! mp4mux ! filesink location=/tmp/01.mp4 In any case, this pipeline generates .mp4 successfully, but without the attached patch, the mp4 file isn't playable. And also I tested the below pipeline. $ $ gst-launch-1.0 multifilesrc location=%03d.jpg caps="image/jpeg,framerate=(fraction)12/1" ! jpegdec ! x264enc ! h264parse config-interval=0 ! mp4mux ! filesink location=/tmp/02.mp4 In this case, the mp4 is playable. Is there wrong point in my test steps?
(In reply to Justin J. Kim from comment #14) > Is there wrong point in my test steps? Sorry, there's mistake. The generated mp4 file is valid only before changing the resolution.
I think the short term answer is to merge the patch here and add a patch to mp4mux & friends that return an error if the codec_data changes, ie returns FALSE on the setcaps which should cause the pipeline to abort with a caps negotiation error.. and possibly post a GstMessage of type error explaining why.
Created attachment 364905 [details] [review] qtmux: send error when refusing caps As Olivier's suggestion, I made a patch to raise an error on the bus if codec_data is changed(the caps is refused to re-negotiation).
Comment on attachment 364905 [details] [review] qtmux: send error when refusing caps Thanks. Is there a reason we can't use the GST_ELEMENT_ERROR() macro here? The first string in the error message should be something that explains the problem to the user without containing technical details such as the pad or the caps. The second part is the 'debug detail string' which can contain all the gory details.
Created attachment 364911 [details] [review] qtmux: send error when refusing caps Use `GST_ELEMENT_ERROR` macro.
Comment on attachment 364911 [details] [review] qtmux: send error when refusing caps Please change the user-visible error message to something like Can't change input format at runtime. And I think GST_ELEMENT_ERROR supports gstreamer debugging format extensions, so you can do "... caps %" GST_PTR_FORMAT, ..., caps); to print caps. Bonus points for printing the old caps as well if they are easily accessible somewhere.
Created attachment 365178 [details] [review] qtmux: send error when refusing caps Changed messages.
Comment on attachment 365178 [details] [review] qtmux: send error when refusing caps Looks fine now, thanks. Only problem is that it breaks the splitmux unit test, so someone needs to investigate if we need to be more liberal in what changes we accept or if the test needs changing.
I think it's very difficult to fix splitmux unit test unless qtmux supports reset. splitmuxsink tries to use different filename when it gets caps event. However, even though it has different files, the file after getting caps event isn't valid - not playable. Can't we disable the unit test for a moment until qtmux really supports the case?
Created attachment 365473 [details] [review] tests: smplitmux: disable caps change I think disabling the case in splitmux test is easier way at the moment.
(In reply to Justin J. Kim from comment #23) > I think it's very difficult to fix splitmux unit test unless qtmux supports > reset. > > splitmuxsink tries to use different filename when it gets caps event. > However, even though it has different files, the file after getting caps > event isn't valid - not playable. Can't we disable the unit test for a > moment until qtmux really supports the case? I fixed that in 3a813a0d a few weeks ago - are you testing latest git master, or is it broken in a different way now?
(In reply to Olivier Crête from comment #12) > (In reply to Sebastian Dröge (slomo) from comment #11) > > > I know this > > > will be a problem for mp4 file, in that case, h264parse should probably ... > Afaik, in the AVC1 case, it should renumber and keep them all around until > the parser/depayloader is reset to READY as this is the only way to make a > truly valid stream. As I think having "filesrc ! tsdemux! h264parse ! mp4mux > ! filesink" should produce a single continuous file even if you do flushing > seeks. If you renumber SPS/PPS, you also need to change every packet that contains a reference to the SPS and PPS ids. In any case, that's not how AVC in mp4 works - it's quite OK to change SPS/PPS on the fly, but the muxer needs to append a new configuration section and avcC entry, and change the STSD entry for the samples so the correct SPS/PPS will be activated when that happens. qtmux doesn't implement that, but qtdemux does - see gst_qtdemux_stream_check_and_change_stsd_index() in qtdemux.c.
(In reply to Jan Schmidt from comment #25) > I fixed that in 3a813a0d a few weeks ago - are you testing latest git > master, or is it broken in a different way now? splitmux test is failed here because I changed to send an error instead of warning in qtmux.
(In reply to Jan Schmidt from comment #26) > If you renumber SPS/PPS, you also need to change every packet that contains > a reference to the SPS and PPS ids. In any case, that's not how AVC in mp4 > works - it's quite OK to change SPS/PPS on the fly, but the muxer needs to > append a new configuration section and avcC entry, and change the STSD entry > for the samples so the correct SPS/PPS will be activated when that happens. > > qtmux doesn't implement that, but qtdemux does - see > gst_qtdemux_stream_check_and_change_stsd_index() in qtdemux.c. That seems much easier, so we should implement that in qtmux I think. My knowledtge of the isobff format is quite limited. I wonder what we can do for flvmux (ie what do Flash & YouTube support) and what about Matroska?
Realistically we are not going to be able to implement this in any muxer taking AVC format, certainly not as part of this bug. It's non-trivial and will be hard to test as well.
*** Bug 792568 has been marked as a duplicate of this bug. ***
(In reply to Tim-Philipp Müller from comment #29) > Realistically we are not going to be able to implement this in any muxer > taking AVC format, certainly not as part of this bug. It's non-trivial and > will be hard to test as well. What are you suggesting? Just applying the patches from here and be done done with it and open another bug for the extra features in qtmux ?
I think we need to figure out what to do about qtmux refusing caps. Jan argued that we can't make qtmux post an error message in this case because that would change behaviour in a backwards compatible way (e.g. it breaks the splitmux unit test). But no alternative solution was offered either. I'm not sure yet how much this convinces me. So I don't know what to do about the muxers currently.
Why doesn't splitmuxsink just create a new file on caps change? That would ensure that qtmux never posts that error as it would never get a caps change in running state.
Oh, I get it, new suggestion, downgrade the error to a warning.. and add some code in splitmuxsink to ignore this warning while trying to change caps.
Created attachment 366902 [details] [review] splitmuxsink: Ignore errors while changing caps Catch and ignore the error whle changing caps, also downgrade the error to a warning as the source will stop anyway with another error afterwards.
For information, the rtph264depay patch here also fixes my issue (https://bugzilla.gnome.org/show_bug.cgi?id=792568) which confirms it was the same. Thanks Olivier for letting me know !
Attachment 364616 [details] pushed as dabeed5 - rtph264depay: update output caps regardless format Attachmewnt 365178 and 366902 squashed as ad8a6cb6399 - qtmux: send stream warning when refusing video caps
For qtmux, what we could also do is: instead of posting an error when refusing caps change in setcaps, we could simply set a flag, and return FLOW_NOT_NEGOTIATED with an error message once we get a buffer on that pad. That way we preserve old behaviour but also error out.