| Message ID | 20170508184625.7056-2-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Commit | 6505e8cfd02b9112e24bb40c145d6c760f15d622 |
| Headers | show |
On Mon, May 08, 2017 at 03:46:23PM -0300, James Almer wrote: > From: Anton Khirnov <anton@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 ? [...]
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@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).
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@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 [...]
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@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.
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@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 [...]
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@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.
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@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.
On 5/20/2017 1:55 PM, James Almer wrote: > 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@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. Pushed, thanks.
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; } h->avctx->coded_width = h->width; diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c index 35ab51f616..a8d07df1e7 100644 --- a/libavcodec/h264dec.c +++ b/libavcodec/h264dec.c @@ -309,6 +309,9 @@ static int h264_init_context(AVCodecContext *avctx, H264Context *h) h->avctx = avctx; h->cur_chroma_format_idc = -1; + h->width_from_caller = avctx->width; + h->height_from_caller = avctx->height; + h->picture_structure = PICT_FRAME; h->workaround_bugs = avctx->workaround_bugs; h->flags = avctx->flags; diff --git a/libavcodec/h264dec.h b/libavcodec/h264dec.h index 1c0dfbf7f7..5e03d55558 100644 --- a/libavcodec/h264dec.h +++ b/libavcodec/h264dec.h @@ -534,6 +534,11 @@ typedef struct H264Context { int cur_bit_depth_luma; int16_t slice_row[MAX_SLICES]; ///< to detect when MAX_SLICES is too low + /* original AVCodecContext dimensions, used to handle container + * cropping */ + int width_from_caller; + int height_from_caller; + int enable_er; H264SEIContext sei;
From: Anton Khirnov <anton@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(-)