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

[Andreas Cadhalpun]

On 14.06.2015 13:46, Luca Barbato wrote:
> On 14/06/15 12:38, Andreas Cadhalpun wrote:
>> I discussed it with Hendrik and asked him for a better way to fix the
>> problem. It seems he hasn't found one, yet.
> 
> Told you how to that, did you miss that part from the previous email?

If you mean just documenting the problem, that won't fix anything in the
short term.

>> Did I just hear you volunteer to check all API users and send patches,
>> where you find it necessary?
> 
> As if hadn't been done in the past... It is not exceedingly fun but me,
> Anton and quite a bit of people from Libav did for the previous API
> migrations.

Yes, that was fixing build failures with newer Libav versions.

However, this issue doesn't cause build failures, but can cause runtime
problems. Finding (and then fixing) problematic code is thus more difficult.

Are you (or someone else here) actually going to do that?

If so, I'd be fine with dropping the patch, but fixing it in libavcodec
looks like a lot less effort.

>> As you can see, it uses the pixel format from the codec context and
>> it doesn't check for width/height/format inconsistencies.
>> I'd say that's the opposite of "hadn't happened here". ;)
> 
> The codec is supposed to do whatever it wants with those fields, you
> have the problem not when you assemble the frame but when you output it
> since reordering would make the context reflect a perceived future than
> the present frame.

The problem is calling av_image_copy with inconsistent parameters.
This happens in above code just as it does in the demuxing_decoding example.

> The lines above mark that as a not exactly stellar solution happening as
> best effort solution.
> 
> What I'd avoid to do again in this discussion =)
> 
> Probably might be safer to check that the reference is matching and use
> black instead, thanks for spotting something that might be an issue.

You're welcome.

>> If that's the case, Michael's idea of moving this backup/restore into
>> avcodec_decode_video2 and using it for codecs declaring some FF_CODEC_CAP_*
>> (or maybe just unconditionally) looks like the way to go.
> 
> Depends if you consider avcodec_decode_video2 a fastpath or a slowpath
> and if those load+store are impacting I guess.

I think those wouldn't impact performance in a noticeable way.

> The least wrong way is to consider those fields as write only in both
> direction and keep a shadow.
> 
> basically
> 
> ## open
> Save the hint values in some internal variables
> 
> avctx->internal->{*width,*height,format} = avctx->{*width,*height,format}
> 
> ## decode
> 
> Use the internal variables as currently is done
> avctx->internal->{*width,*height,format}
> 
> Update the external
> avctx->{*width,*height,format} is updated using the avframe.

Yes, that would work as well and if one would design the API from scratch
this way would be preferable.

> I hope now it is clear how I'd fix it and why I consider it too daunting
> given the non-problem it supposedly solves.

It would require changing a lot of code to use the internal values, but
I think the same effect could be achieved by backing up/restoring the values.
And that would require significantly less code changes.

Best regards,
Andreas