[FFmpeg-devel] [PATCH 2/4] h264dec: be more explicit in handling container cropping

[James Almer]

On 5/9/2017 11:56 PM, Michael Niedermayer wrote:
> On Mon, May 08, 2017 at 03:46:23PM -0300, James Almer wrote:
>> From: Anton Khirnov <anton at khirnov.net>
>>
>> The current condition can trigger in cases where it shouldn't, with
>> unexpected results.
>> Make sure that:
>> - container cropping is really based on the original dimensions from the
>>   caller
>> - those dimenions are discarded on size change
>>
>> The code is still quite hacky and eventually should be deprecated and
>> removed, with the decision about which cropping is used delegated to the
>> caller.
>> ---
>> This merges commit 4fded0480f20f4d7ca5e776a85574de34dfead14 from libav
>>
>>  libavcodec/h264_slice.c | 20 +++++++++++++-------
>>  libavcodec/h264dec.c    |  3 +++
>>  libavcodec/h264dec.h    |  5 +++++
>>  3 files changed, 21 insertions(+), 7 deletions(-)
>>
>> diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
>> index acf6a73f60..a7916e09ce 100644
>> --- a/libavcodec/h264_slice.c
>> +++ b/libavcodec/h264_slice.c
>> @@ -378,6 +378,8 @@ int ff_h264_update_thread_context(AVCodecContext *dst,
>>      h->avctx->coded_width   = h1->avctx->coded_width;
>>      h->avctx->width         = h1->avctx->width;
>>      h->avctx->height        = h1->avctx->height;
>> +    h->width_from_caller    = h1->width_from_caller;
>> +    h->height_from_caller   = h1->height_from_caller;
>>      h->coded_picture_number = h1->coded_picture_number;
>>      h->first_field          = h1->first_field;
>>      h->picture_structure    = h1->picture_structure;
> 
>> @@ -874,13 +876,17 @@ static int init_dimensions(H264Context *h)
>>      av_assert0(sps->crop_top + sps->crop_bottom < (unsigned)h->height);
>>  
>>      /* handle container cropping */
>> -    if (FFALIGN(h->avctx->width,  16) == FFALIGN(width,  16) &&
>> -        FFALIGN(h->avctx->height, 16) == FFALIGN(height, 16) &&
>> -        h->avctx->width  <= width &&
>> -        h->avctx->height <= height
>> -    ) {
>> -        width  = h->avctx->width;
>> -        height = h->avctx->height;
>> +    if (h->width_from_caller > 0 && h->height_from_caller > 0     &&
>> +        !sps->crop_top && !sps->crop_left                         &&
>> +        FFALIGN(h->width_from_caller,  16) == FFALIGN(width,  16) &&
>> +        FFALIGN(h->height_from_caller, 16) == FFALIGN(height, 16) &&
>> +        h->width_from_caller  <= width &&
>> +        h->height_from_caller <= height) {
>> +        width  = h->width_from_caller;
>> +        height = h->height_from_caller;
>> +    } else {
>> +        h->width_from_caller  = 0;
>> +        h->height_from_caller = 0;
>>      }
> 
> With this, seeking in a file could affect if croping is used
> would something break if croping was unaffected by what was priorly
> decoded ?

I don't know. Do you have a test case where this could break that i can
check?

I could skip this patch, seeing it's one of the points where it didn't
apply cleanly. Perhaps the faulty condition the commit message mentioned
was already dealt with on our side.
The next patch still works and fate passes if i only add the
"!sps->crop_top && !sps->crop_left" checks from this patch (it fails
without them).