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

[James Almer]

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'd like to apply the set soon so we can resume the merges, so if there
are doubts about this patch i can skip it and add it to
unfortunately-still-growing skipped merges list to be implemented later.