GNOME Bugzilla – Bug 719359
vp8dec: Doesn't handle changes in resolution
Last modified: 2014-09-02 06:14:23 UTC
Created attachment 262856 [details] [review] Patch that applies to 1.2.1 When decoding VP8 it's possible for the stream to change resolution mid-stream which currently doesn't work as the caps are not changed. This can lead to a band of corruption especially along the bottom or side of the video. Changes in resolution typically happen with streams generated by tablets or phones as rotation causes the camera to change aspect ratio. The patch attached is fairly straightforward in that it calls gst_video_decoder_set_output_state if the output image doesn't match the size of the current output state.
Review of attachment 262856 [details] [review]: Basically good, needs to be applied to vp9dec too which is more or less the same code. ::: ext/vpx/gstvp8dec.c @@ +430,2 @@ g_assert (dec->output_state == NULL); + dec->output_state = gst_video_decoder_set_output_state (GST_VIDEO_DECODER (dec), GST_VIDEO_FORMAT_I420, stream_info.w, stream_info.h, state); /* Use NULL? Why use input_state? */ Please run all this through gst-indent again after your changes, also you changed nothing in this line :) The input state is passed there to copy over things like the pixel-aspect-ratio and the framerate @@ +551,3 @@ + new_output_state = + gst_video_decoder_set_output_state (GST_VIDEO_DECODER (dec), + GST_VIDEO_FORMAT_I420, img->d_w, img->d_h, dec->output_state); Must use the input state here @@ +555,3 @@ + gst_video_codec_state_unref (dec->output_state); + } + dec->output_state = new_output_state; You might want to call gst_video_decoder_negotiate() manually here... not really required, but you could give a more useful error message if that fails than when allocate_output_frame() below fails.
(In reply to comment #1) > Review of attachment 262856 [details] [review]: > > Basically good, needs to be applied to vp9dec too which is more or less the > same code. > > ::: ext/vpx/gstvp8dec.c > @@ +430,2 @@ > g_assert (dec->output_state == NULL); > + dec->output_state = gst_video_decoder_set_output_state (GST_VIDEO_DECODER > (dec), GST_VIDEO_FORMAT_I420, stream_info.w, stream_info.h, state); /* Use > NULL? Why use input_state? */ > > Please run all this through gst-indent again after your changes, also you > changed nothing in this line :) I will remove this bit. I was just going through trying to understand the code really. > > The input state is passed there to copy over things like the pixel-aspect-ratio > and the framerate > > @@ +551,3 @@ > + new_output_state = > + gst_video_decoder_set_output_state (GST_VIDEO_DECODER (dec), > + GST_VIDEO_FORMAT_I420, img->d_w, img->d_h, dec->output_state); > > Must use the input state here I used the output state as the basis as the input_state has been put into the output_state already within open_codec and this code is only used when the size changes. I wanted to make sure that anything that was done to the output_state was preserved as otherwise I thought I might trigger a reset of the decoder and only changes to width/height were applied. Am I guaranteed that nothing else has been put into the output_state that needs to be preserved? Is there a guarantee that the input_state will work out to be the same? The input_state is used in open_codec so any changes in input_state will be preserved there. Basically I want to ensure that as far as possible the state doesn't get changed except for width/height at this point - from the docs that seemed the correct way to go. Are there some guidelines somewhere about what to use? Perhaps the docs for gst_video_decoder_set_output_state should be changed to state that the last parameter should always be the input state? > > @@ +555,3 @@ > + gst_video_codec_state_unref (dec->output_state); > + } > + dec->output_state = new_output_state; > > You might want to call gst_video_decoder_negotiate() manually here... not > really required, but you could give a more useful error message if that fails > than when allocate_output_frame() below fails. I noticed that the error message in gstvideodecoder.c wasn't that great, I actually have changed that code at times to debug better, but I thought it was better to rely on the shared code than to duplicate the call to negotiate here.
I applied this on top of 1.4.0 and it works just fine.
commit 5430b6c351e015265a79eb92fc9203ec8bc06de2 Author: Tom Greenwood <tcdgreenwood@hotmail.com> Date: Tue Oct 22 18:49:22 2013 +0100 vp8dec: Fix for handling resolution changes when decoding VP8 If the resolution changes in the bitstream without the input caps changing we would previously output corrupted video or crash. https://bugzilla.gnome.org/show_bug.cgi?id=719359
Can't be solved for vp9dec currently because libvpx reports wrong width/height
Actually it works commit 9eb22a533b429dc57aca9ba2d7a1b01d4361d98c Author: Sebastian Dröge <sebastian@centricular.com> Date: Tue Sep 2 09:09:49 2014 +0300 vp9dec: Get input width/height from the codec instead of the input caps They are reported properly by libvpx if the correct struct members are used. This also fixes handling of resolution changes without input caps changes. https://bugzilla.gnome.org/show_bug.cgi?id=719359
Related bug #734266