GNOME Bugzilla – Bug 758907
vaapidecode: trigger downstream renegotiation in gst_vaapidecode_set_format()
Last modified: 2016-10-31 14:16:03 UTC
Created attachment 316615 [details] [review] patch For resolution changes currently only gst_vaapi_decoder_state_changed() triggers downstream renegotiation. However, this only works under some circumstances because of a bug: First gst_vaapidecode_set_format() is called: [...] if (!gst_vaapi_decode_input_state_replace (decode, state)) return TRUE; [...] This updates the input state with the caps provided by upstream, most likly the h264 parser. Then gst_vaapi_decoder_state_changed() is called: [...] if (!gst_vaapi_decode_input_state_replace (decode, codec_state)) return; [...] decode->do_renego = TRUE; [...] If the internally generated codec state is identical to the one derived from the upstream caps, then renegotiation is not triggered. For avc streams GstVaapiDecoder caches the codec_data from the initial caps. As a result gst_vaapi_decoder_state_changed() is called with a codec state that contains the wrong codec_data in the caps and gst_vaapi_decode_input_state_replace() returns TRUE. By setting do_renego in gst_vaapidecode_set_format() renegotiation is triggered even if gst_vaapi_decoder_state_changed() correctly detects no state change. I suppose the codec_data should be updated correctly as well, once this issue is fixed. However, I'm not sure how to do this. Maybe some kind of gst_vaapidecode_set_caps() in gst_vaapidecode_reset_full() when the decoder is not recreated?
(In reply to Michael Olbrich from comment #0) First of all, I agreed that there are issues related with resolution change handling :) See this: https://bugzilla.gnome.org/show_bug.cgi?id=753914 > > If the internally generated codec state is identical to the one derived > from the upstream caps, then renegotiation is not triggered. > For avc streams GstVaapiDecoder caches the codec_data from the initial > caps. As a result gst_vaapi_decoder_state_changed() is called with a codec > state that contains the wrong codec_data in the caps and > gst_vaapi_decode_input_state_replace() returns TRUE. I don't know whether I understood it correctly. BTW, If there is no sps/pps stuffed in the stream (avc), GstVaapiDecoder won't trigger gst_vaapi_decode_state_changed(). > > By setting do_renego in gst_vaapidecode_set_format() renegotiation is > triggered even if gst_vaapi_decoder_state_changed() correctly detects no > state change. This is wrong: see the commit log of: 6eba201f3252eba6a99ab7da7a4c662091a3e884 > > I suppose the codec_data should be updated correctly as well, once this > issue is fixed. However, I'm not sure how to do this. Maybe some kind of > gst_vaapidecode_set_caps() in gst_vaapidecode_reset_full() when the decoder > is not recreated? IIUC your point is: the internal decoder(GstVaapiDecoder) is not getting updated with codec_data (and the new resolution too) for the multi-resolution AVC stream. Right? Yup, we need a proper fix here. IMHO, We should consider this issue together with #753914. Could you please attach the stream? But I am afraid this is bit risky to do for the 0.7.0 release.. :)
> > If the internally generated codec state is identical to the one derived > > from the upstream caps, then renegotiation is not triggered. > > For avc streams GstVaapiDecoder caches the codec_data from the initial > > caps. As a result gst_vaapi_decoder_state_changed() is called with a codec > > state that contains the wrong codec_data in the caps and > > gst_vaapi_decode_input_state_replace() returns TRUE. > > I don't know whether I understood it correctly. BTW, If there is no sps/pps > stuffed in the stream (avc), GstVaapiDecoder won't trigger > gst_vaapi_decode_state_changed(). Hmm, I have no idea about the low-level stuff, but at some point the decoder will notice the new size, right? Otherwise it won't be able to decode the stream, or how is this handled? > > > > By setting do_renego in gst_vaapidecode_set_format() renegotiation is > > triggered even if gst_vaapi_decoder_state_changed() correctly detects no > > state change. > > This is wrong: > see the commit log of: 6eba201f3252eba6a99ab7da7a4c662091a3e884 Are you sure? gst_vaapidecode_set_format() won't be called until all buffers with the old format are already queued and pushed. It's triggered by the caps event and that's a serialized event. Actually, I think gst_vaapidecode_set_format() should trigger the renegotiation immediately. Or what am I missing here? > > I suppose the codec_data should be updated correctly as well, once this > > issue is fixed. However, I'm not sure how to do this. Maybe some kind of > > gst_vaapidecode_set_caps() in gst_vaapidecode_reset_full() when the decoder > > is not recreated? > > IIUC your point is: the internal decoder(GstVaapiDecoder) is not getting > updated with codec_data (and the new resolution too) for the > multi-resolution AVC stream. Right? Exactly. > Yup, we need a proper fix here. IMHO, We should consider this issue together > with #753914. > > Could you please attach the stream? I don't have the original stream where I first noticed the problem, but I created an artificial one that can trigger the Problem with the right pipeline. I'll attach it. > But I am afraid this is bit risky to do for the 0.7.0 release.. :) That's ok, I'm building from source anyway, so I can just apply whatever patch we come up with.
(In reply to Michael Olbrich from comment #2) > > > If the internally generated codec state is identical to the one derived > > > from the upstream caps, then renegotiation is not triggered. > > > For avc streams GstVaapiDecoder caches the codec_data from the initial > > > caps. As a result gst_vaapi_decoder_state_changed() is called with a codec > > > state that contains the wrong codec_data in the caps and > > > gst_vaapi_decode_input_state_replace() returns TRUE. > > > > I don't know whether I understood it correctly. BTW, If there is no sps/pps > > stuffed in the stream (avc), GstVaapiDecoder won't trigger > > gst_vaapi_decode_state_changed(). > > Hmm, I have no idea about the low-level stuff, but at some point the decoder > will notice the new size, right? Otherwise it won't be able to decode the > stream, or how is this handled? If the incoming stream is byte-stream, the internal decoder(GstVaapiDEcoder) will get the sps header as a part of elementary bitstream , so the decoder will parse the header and eventually will get to know the new resolution too. For AVC stream, the sps header could be only the part of codec_data. We only send codec_data to GstVaapiDecoder in the first time. After that we are not providing the new codec_data from plugin (vaapidecode) to core libgstvaapi(GstVaapiDecoder) . IIUC this is exactly what you articulated too . Thanks for finding this :)
(In reply to Michael Olbrich from comment #2) > > > By setting do_renego in gst_vaapidecode_set_format() renegotiation is > > > triggered even if gst_vaapi_decoder_state_changed() correctly detects no > > > state change. > > > > This is wrong: > > see the commit log of: 6eba201f3252eba6a99ab7da7a4c662091a3e884 > > Are you sure? gst_vaapidecode_set_format() won't be called until all buffers > with the old format are already queued and pushed. It's triggered by the caps > event and that's a serialized event. > > Actually, I think gst_vaapidecode_set_format() should trigger the > renegotiation immediately. > > Or what am I missing here? This is bit tricky :), which I explained in the commit 6eba201f3252eba6a99ab7da7a4c662091a3e884 This became tricky because of the design of gstreamer-vaaapi. Parser will not report the resolution change always: eg: suppose the parser receive first sps header having resolution 1920x1088 and crop_rectangle 1920x1080, then h264parse will report resolution 1920x1080... Then after couple of frames, a new sps came with resolution 1920x1080 (no cropping), here the h264parse will not invoke the set_caps since the effective resolution is the same :). So in this scenario we will not receive a set_format() invoke in gstvaapidecode.c...
(In reply to sreerenj from comment #4) > (In reply to Michael Olbrich from comment #2) > > > > > By setting do_renego in gst_vaapidecode_set_format() renegotiation is > > > > triggered even if gst_vaapi_decoder_state_changed() correctly detects no > > > > state change. > > > > > > This is wrong: > > > see the commit log of: 6eba201f3252eba6a99ab7da7a4c662091a3e884 > > > > Are you sure? gst_vaapidecode_set_format() won't be called until all buffers > > with the old format are already queued and pushed. It's triggered by the caps > > event and that's a serialized event. > > > > Actually, I think gst_vaapidecode_set_format() should trigger the > > renegotiation immediately. > > > > Or what am I missing here? > > > This is bit tricky :), which I explained in the commit > 6eba201f3252eba6a99ab7da7a4c662091a3e884 > > This became tricky because of the design of gstreamer-vaaapi. BTW, Some points about the memory allocation and renegotiation is here: https://bugzilla.gnome.org/show_bug.cgi?id=752958#c6 Actual surface creation is happening in core libgstvaapi (gst-libs/gst/vaapi) not in plugins(gst/vaapi)
Created attachment 316659 [details] test video
Created attachment 316672 [details] better test video this one works better
I've looked at this some more, and I think, my patch is correct: When gst_vaapidecode_set_format() is called for the new format some frames may still be queued for for the old format but all data for the old format has arrived before. -> gst_vaapi_decode_input_state_replace() sets the new state. Then the First data for the new format arrives (starting with the SPS) and gst_vaapi_decoder_state_changed() is called. -> gst_vaapi_decode_input_state_replace() returns false -> do_renego is _not_ set. After that gst_vaapidecode_handle_frame() will push all old frames but no negotiation happens. By setting do_renego in gst_vaapidecode_set_format() downstream negotiation will happen, but not until all old frames are pushed downstream.
(In reply to Michael Olbrich from comment #7) > Created attachment 316672 [details] > better test video > > this one works better This stream is "byte-stream" elementary stream ,, not avc... The issue you have reported should be possible with avc streams only..
(In reply to Michael Olbrich from comment #8) > I've looked at this some more, and I think, my patch is correct: > > When gst_vaapidecode_set_format() is called for the new format some frames > may still be queued for for the old format but all data for the old format > has arrived before. > -> gst_vaapi_decode_input_state_replace() sets the new state. > > Then the First data for the new format arrives (starting with the SPS) and > gst_vaapi_decoder_state_changed() is called. > -> gst_vaapi_decode_input_state_replace() returns false > -> do_renego is _not_ set. > > After that gst_vaapidecode_handle_frame() will push all old frames but no > negotiation happens. > > By setting do_renego in gst_vaapidecode_set_format() downstream negotiation > will happen, but not until all old frames are pushed downstream. Patch should be fine, but i don't think it is fixing the actual issue. We should communicate the avc-sps/pss (codec_data) to ligstvaapi(gst-libs/gst/vaapi). On a different note, the renegotiation strategy for VP9 streams is completely different from all other codecs (mpeg2, h264, h265 etc).. We have to consider all of these cases together for fixing this...
Postponed for 0.8.0...
(In reply to sreerenj from comment #9) > This stream is "byte-stream" elementary stream ,, not avc... > The issue you have reported should be possible with avc streams only.. The issue I reported is independent of stream-format. Maybe my initial report was not quite clear about that. The original issue is this: For any H.264 pipeline with the following conditions: 1. The stream contains the correct SPS/PPS on resolution changes 2. The parser before the decoder is not in pass-through mode so it generates new caps events for each resolution change. What happens right now is, that 1. gst_vaapidecode_set_format() calls gst_vaapi_decode_input_state_replace() 2. gst_vaapi_decoder_state_changed() will detect no changes -> do_renego is not set and no downstream negotiation happens. There is one exception here: For AVC streams the the codec_state in gst_vaapi_decoder_state_changed() contains the codec_data of the initial caps. As a result gst_vaapi_decode_input_state_replace() detects a state change. Try this pipeline with the test video: gst-launch-1.0 -v filesrc location=rottest.ts ! tsdemux ! h264parse ! video/x-h264,stream-format=avc ! vaapidecode ! vaapisink You'll notice that the first change works (different codec_data) but not the second (back to the original codec_data, so no change). With stream-format=byte-stream,alignment=au neither change works correctly. I hope this make the original issue and what the patch fixes clearer.
(In reply to Michael Olbrich from comment #12) > (In reply to sreerenj from comment #9) I understood your point, but wanna fix/push all related fixes together :) > > This stream is "byte-stream" elementary stream ,, not avc... > > The issue you have reported should be possible with avc streams only.. > > The issue I reported is independent of stream-format. Maybe my initial > report was > not quite clear about that. > > The original issue is this: > For any H.264 pipeline with the following conditions: > 1. The stream contains the correct SPS/PPS on resolution changes > 2. The parser before the decoder is not in pass-through mode so it generates > new caps events for each resolution change. There are multiple cases needs to be addressed: 1: The case I explained in comment_4 (in theory not an issue, but design of gstremaer-vaapi won't work with this) 2:Passthrough-mode enablement in upstream parser is wrong (https://bugzilla.gnome.org/show_bug.cgi?id=758656) 3: All the resolution changes doesn't mean to do buffer-pool renegotiation... We need a way to set the cropped width/height (or the actual resolution) in src caps with out doing pool renegotiation . I will get back to this after 0.7.0 release.... > > What happens right now is, that > 1. gst_vaapidecode_set_format() calls gst_vaapi_decode_input_state_replace() > 2. gst_vaapi_decoder_state_changed() will detect no changes > -> do_renego is not set and no downstream negotiation happens. This is Right :) >
Please test with the current git master and let us know if issue persists. git://anongit.freedesktop.org/gstreamer/gstreamer-vaapi
Closing, free free to reopen if issue persists.