[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

CVFC1_Sony_C.jsv is fixed now that the cropping logic works correctly.
tickets/4274/sample.ts still shows the difference, but it changes
garbage output with slightly different, less ugly but still garbage output.

Old: http://0x0.st/ghF.png
New: http://0x0.st/ghC.png

Unless this can be reproduced with negative effects with a sane file and
not with a badly cut mpegts stream with missing references that requires
seeking and a some specific flags, i'm inclined to not consider it worth
bothering with.

I'll be pushing the set sometime next week if no other regressions are
found.