[Bug 719359] vp8dec: Doesn't handle changes in resolution

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=719359
  GStreamer | gst-plugins-good | 1.0.10

--- Comment #2 from tcdgreenwood at hotmail.com 2014-02-25 19:14:09 UTC ---
(In reply to comment #1)
> Review of attachment 262856 [details]:
> 
> 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.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.