After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 719359 - vp8dec: Doesn't handle changes in resolution
vp8dec: Doesn't handle changes in resolution
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.0.10
Other All
: Normal major
: 1.4.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-11-26 14:05 UTC by tcdgreenwood
Modified: 2014-09-02 06:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch that applies to 1.2.1 (2.43 KB, patch)
2013-11-26 14:05 UTC, tcdgreenwood
committed Details | Review

Description tcdgreenwood 2013-11-26 14:05:33 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.
Comment 1 Sebastian Dröge (slomo) 2013-12-05 15:15:05 UTC
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.
Comment 2 tcdgreenwood 2014-02-25 19:14:09 UTC
(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.
Comment 3 Robert Swain 2014-08-29 12:35:40 UTC
I applied this on top of 1.4.0 and it works just fine.
Comment 4 Sebastian Dröge (slomo) 2014-09-02 05:44:33 UTC
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
Comment 5 Sebastian Dröge (slomo) 2014-09-02 06:05:56 UTC
Can't be solved for vp9dec currently because libvpx reports wrong width/height
Comment 6 Sebastian Dröge (slomo) 2014-09-02 06:11:07 UTC
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
Comment 7 Sebastian Dröge (slomo) 2014-09-02 06:14:12 UTC
Related bug #734266