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

[James Almer]

On 5/11/2017 9:56 AM, Michael Niedermayer wrote:
> On Wed, May 10, 2017 at 11:06:52PM -0300, James Almer wrote:
>> On 5/10/2017 9:55 PM, Michael Niedermayer wrote:
>>> On Wed, May 10, 2017 at 10:41:30AM -0300, James Almer wrote:
>>>> 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?
>>>
>>> no, it was just an thought that came to my mind when looking at the
>>> code. I dont remember seeing this break anything, it changed some
>>> files output though unless these were caused by another patch i had
>>> locally
>>
>> Could you try to confirm they weren't changed by this patch? Unless i'm
>> reading it wrong, this set afaik isn't supposed to change the output of
>> the decoder (at least not negatively), as reflected by fate, just move
>> the cropping logic to decode.c
> 
> I retested, it was
> [3/4] h264dec: export cropping information instead of handling it internally
> that caused the changes
> changes seen are with CVFC1_Sony_C.jsv and tickets/4274/sample.ts
> 
> 4247 needs "-threads 1  -flags2 showall -ss 7" for showin the
> difference, the sony file shows a difference with just plain default
> reencoding to avi
> 
> Our fate test doesnt change, i guess due to -flags unaligned in it
> 
> thx

The decode.c logic is different than the decoder specific one when the
unaligned flag is not set, and it's cropping videos where the latter didn't.

I don't know if it's better this way or not, so I'm skipping these
commits for the time being and leaving this to someone more familiar
with the code, or until someone can confirm this is in fact intended.