[FFmpeg-devel] [PATCH] h264: update avctx width/height when flushing

[Michael Niedermayer]

On Wed, Jun 10, 2015 at 09:10:31PM +0200, Andreas Cadhalpun wrote:
> On 10.06.2015 12:01, Michael Niedermayer wrote:
> > On Wed, Jun 10, 2015 at 11:43:16AM +0200, Michael Niedermayer wrote:
> >> On Tue, Jun 09, 2015 at 11:53:03PM +0200, Andreas Cadhalpun wrote:
> >>> Inconsistencies between the dimensions of avctx and the frame can
> >>> confuse API users. For example this can crash the demuxing_decoding
> >>> example.
> >>>
> >>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> >>> ---
> >>>  libavcodec/h264.c | 4 ++++
> >>>  1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/libavcodec/h264.c b/libavcodec/h264.c
> >>> index 9a00214..d1eaa5e 100644
> >>> --- a/libavcodec/h264.c
> >>> +++ b/libavcodec/h264.c
> >>> @@ -1757,6 +1757,8 @@ static int h264_decode_frame(AVCodecContext *avctx, void *data,
> >>>              if (ret < 0)
> >>>                  return ret;
> >>>              *got_frame = 1;
> >>> +            avctx->width  = pict->width;
> >>> +            avctx->height = pict->height;
> >>>          }
> >>
> >> iam not sure this is not breaking something, other parts of the h264
> >> decoder expect this to be set relative to the last frame in coding
> >> order.
> 
> If that's the case, then I think the h264 decoder should use a field
> in the private H264Context for that information and not the public
> fields in AVCodecContext.
> 
> >> also avctx->pix_fmt would then still potentially not match the last
> >> output frame, also pix_fmt has the same problem as above
> 
> Yes, pix_fmt should also be updated. Attached patch does this, but

> this caused changes in some h264 reinit fate tests. Is that a problem?

looking at the testfile with ffplay shows heavy artifacts after the
patch and no artifacts before


> 
> >> ive fixed one use of width/height not to depend on the AVCodecContext
> >> value but a 2nd remains
> > 
> > actually theres more code that uses it
> > ff_h264_draw_horiz_band() and its callers use h->avctx->height too
> 
> Could these be fixed to use a new H264Context field instead?

i think it might be possible for them to use the AVFrame height
but the code should be tested
especially with a file that uses croping and some application that
uses these codepathes
iam not sure this codepath works currently with files using croping
and using the AVFrame height and if needed an adjusted pointer
offset might fix that or might break it depending on what applications
expect

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Old school: Use the lowest level language in which you can solve the problem
            conveniently.
New school: Use the highest level language in which the latest supercomputer
            can solve the problem without the user falling asleep waiting.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150610/5f11f543/attachment.asc>