[FFmpeg-devel] [PATCH 2/4] h264dec: be more explicit in handling container cropping
Michael Niedermayer
michael at niedermayer.cc
Thu May 11 15:56:21 EEST 2017
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
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
If you think the mosad wants you dead since a long time then you are either
wrong or dead since a long time.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170511/bdea8dd8/attachment.sig>
More information about the ffmpeg-devel
mailing list