| Message ID | 20201017193020.245-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/av1dec: fix loading PrevGmParams for frames with primary_ref_frame none | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | warning | Make fate failed |
On 17/10/2020 20:30, James Almer wrote: > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/av1dec.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c > index 54aeba1812..04aaf5d148 100644 > --- a/libavcodec/av1dec.c > +++ b/libavcodec/av1dec.c > @@ -109,13 +109,18 @@ static void read_global_param(AV1DecContext *s, int type, int ref, int idx) > { > uint8_t primary_frame, prev_frame; > uint32_t abs_bits, prec_bits, round, prec_diff, sub, mx; > - int32_t r; > + int32_t r, prev_gm_param; > > primary_frame = s->raw_frame_header->primary_ref_frame; > prev_frame = s->raw_frame_header->ref_frame_idx[primary_frame]; > abs_bits = AV1_GM_ABS_ALPHA_BITS; > prec_bits = AV1_GM_ALPHA_PREC_BITS; > > + if (s->raw_frame_header->primary_ref_frame == AV1_PRIMARY_REF_NONE) > + prev_gm_param = s->cur_frame.gm_params[ref][idx]; To clarify that I'm reading the standard correctly here, this is the value set for PrevGmParams by setup_past_independance(), which then matches the default for gm_params at the top of global_motion_params() so you've reused it here? > + else > + prev_gm_param = s->ref[prev_frame].gm_params[ref][idx]; > + > if (idx < 2) { > if (type == AV1_WARP_MODEL_TRANSLATION) { > abs_bits = AV1_GM_ABS_TRANS_ONLY_BITS - > @@ -131,7 +136,7 @@ static void read_global_param(AV1DecContext *s, int type, int ref, int idx) > prec_diff = AV1_WARPEDMODEL_PREC_BITS - prec_bits; > sub = (idx % 3) == 2 ? (1 << prec_bits) : 0; > mx = 1 << abs_bits; > - r = (s->ref[prev_frame].gm_params[ref][idx] >> prec_diff) - sub; > + r = (prev_gm_param >> prec_diff) - sub; > > s->cur_frame.gm_params[ref][idx] = > (decode_signed_subexp_with_ref(s->raw_frame_header->gm_params[ref][idx], > - Mark
On 10/27/2020 5:57 PM, Mark Thompson wrote: > On 17/10/2020 20:30, James Almer wrote: >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavcodec/av1dec.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c >> index 54aeba1812..04aaf5d148 100644 >> --- a/libavcodec/av1dec.c >> +++ b/libavcodec/av1dec.c >> @@ -109,13 +109,18 @@ static void read_global_param(AV1DecContext *s, >> int type, int ref, int idx) >> { >> uint8_t primary_frame, prev_frame; >> uint32_t abs_bits, prec_bits, round, prec_diff, sub, mx; >> - int32_t r; >> + int32_t r, prev_gm_param; >> primary_frame = s->raw_frame_header->primary_ref_frame; >> prev_frame = s->raw_frame_header->ref_frame_idx[primary_frame]; >> abs_bits = AV1_GM_ABS_ALPHA_BITS; >> prec_bits = AV1_GM_ALPHA_PREC_BITS; >> + if (s->raw_frame_header->primary_ref_frame == >> AV1_PRIMARY_REF_NONE) >> + prev_gm_param = s->cur_frame.gm_params[ref][idx]; > > To clarify that I'm reading the standard correctly here, this is the > value set for PrevGmParams by setup_past_independance(), which then > matches the default for gm_params at the top of global_motion_params() > so you've reused it here? Correct. Assuming s->cur_frame.gm_params will contain the default values when primary_ref_frame == AV1_PRIMARY_REF_NONE (as it happens in global_motion_params()) simplifies the code quite a bit. Otherwise I'd have to add some array with said defaults somewhere. > >> + else >> + prev_gm_param = s->ref[prev_frame].gm_params[ref][idx]; >> + >> if (idx < 2) { >> if (type == AV1_WARP_MODEL_TRANSLATION) { >> abs_bits = AV1_GM_ABS_TRANS_ONLY_BITS - >> @@ -131,7 +136,7 @@ static void read_global_param(AV1DecContext *s, >> int type, int ref, int idx) >> prec_diff = AV1_WARPEDMODEL_PREC_BITS - prec_bits; >> sub = (idx % 3) == 2 ? (1 << prec_bits) : 0; >> mx = 1 << abs_bits; >> - r = (s->ref[prev_frame].gm_params[ref][idx] >> prec_diff) - sub; >> + r = (prev_gm_param >> prec_diff) - sub; >> s->cur_frame.gm_params[ref][idx] = >> >> (decode_signed_subexp_with_ref(s->raw_frame_header->gm_params[ref][idx], >> > > - Mark > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On 27/10/2020 21:01, James Almer wrote: > On 10/27/2020 5:57 PM, Mark Thompson wrote: >> On 17/10/2020 20:30, James Almer wrote: >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> libavcodec/av1dec.c | 9 +++++++-- >>> 1 file changed, 7 insertions(+), 2 deletions(-) >>> >>> diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c >>> index 54aeba1812..04aaf5d148 100644 >>> --- a/libavcodec/av1dec.c >>> +++ b/libavcodec/av1dec.c >>> @@ -109,13 +109,18 @@ static void read_global_param(AV1DecContext *s, >>> int type, int ref, int idx) >>> { >>> uint8_t primary_frame, prev_frame; >>> uint32_t abs_bits, prec_bits, round, prec_diff, sub, mx; >>> - int32_t r; >>> + int32_t r, prev_gm_param; >>> primary_frame = s->raw_frame_header->primary_ref_frame; >>> prev_frame = s->raw_frame_header->ref_frame_idx[primary_frame]; >>> abs_bits = AV1_GM_ABS_ALPHA_BITS; >>> prec_bits = AV1_GM_ALPHA_PREC_BITS; >>> + if (s->raw_frame_header->primary_ref_frame == >>> AV1_PRIMARY_REF_NONE) >>> + prev_gm_param = s->cur_frame.gm_params[ref][idx]; >> >> To clarify that I'm reading the standard correctly here, this is the >> value set for PrevGmParams by setup_past_independance(), which then >> matches the default for gm_params at the top of global_motion_params() >> so you've reused it here? > > Correct. Assuming s->cur_frame.gm_params will contain the default values > when primary_ref_frame == AV1_PRIMARY_REF_NONE (as it happens in > global_motion_params()) simplifies the code quite a bit. Otherwise I'd > have to add some array with said defaults somewhere. Do you mind adding a little comment saying that? It took me quite a few minutes of staring at it thinking it was wrong to realise what was going on. >> >>> + else >>> + prev_gm_param = s->ref[prev_frame].gm_params[ref][idx]; >>> + >>> if (idx < 2) { >>> if (type == AV1_WARP_MODEL_TRANSLATION) { >>> abs_bits = AV1_GM_ABS_TRANS_ONLY_BITS - >>> @@ -131,7 +136,7 @@ static void read_global_param(AV1DecContext *s, >>> int type, int ref, int idx) >>> prec_diff = AV1_WARPEDMODEL_PREC_BITS - prec_bits; >>> sub = (idx % 3) == 2 ? (1 << prec_bits) : 0; >>> mx = 1 << abs_bits; >>> - r = (s->ref[prev_frame].gm_params[ref][idx] >> prec_diff) - sub; >>> + r = (prev_gm_param >> prec_diff) - sub; >>> s->cur_frame.gm_params[ref][idx] = >>> >>> (decode_signed_subexp_with_ref(s->raw_frame_header->gm_params[ref][idx], >>> LGTM with that. Thanks, - Mark
On 10/27/2020 6:04 PM, Mark Thompson wrote: > On 27/10/2020 21:01, James Almer wrote: >> On 10/27/2020 5:57 PM, Mark Thompson wrote: >>> On 17/10/2020 20:30, James Almer wrote: >>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>> --- >>>> libavcodec/av1dec.c | 9 +++++++-- >>>> 1 file changed, 7 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c >>>> index 54aeba1812..04aaf5d148 100644 >>>> --- a/libavcodec/av1dec.c >>>> +++ b/libavcodec/av1dec.c >>>> @@ -109,13 +109,18 @@ static void read_global_param(AV1DecContext *s, >>>> int type, int ref, int idx) >>>> { >>>> uint8_t primary_frame, prev_frame; >>>> uint32_t abs_bits, prec_bits, round, prec_diff, sub, mx; >>>> - int32_t r; >>>> + int32_t r, prev_gm_param; >>>> primary_frame = s->raw_frame_header->primary_ref_frame; >>>> prev_frame = s->raw_frame_header->ref_frame_idx[primary_frame]; >>>> abs_bits = AV1_GM_ABS_ALPHA_BITS; >>>> prec_bits = AV1_GM_ALPHA_PREC_BITS; >>>> + if (s->raw_frame_header->primary_ref_frame == >>>> AV1_PRIMARY_REF_NONE) >>>> + prev_gm_param = s->cur_frame.gm_params[ref][idx]; >>> >>> To clarify that I'm reading the standard correctly here, this is the >>> value set for PrevGmParams by setup_past_independance(), which then >>> matches the default for gm_params at the top of global_motion_params() >>> so you've reused it here? >> >> Correct. Assuming s->cur_frame.gm_params will contain the default values >> when primary_ref_frame == AV1_PRIMARY_REF_NONE (as it happens in >> global_motion_params()) simplifies the code quite a bit. Otherwise I'd >> have to add some array with said defaults somewhere. > > Do you mind adding a little comment saying that? It took me quite a few > minutes of staring at it thinking it was wrong to realise what was going > on. Sure. And sorry. > >>> >>>> + else >>>> + prev_gm_param = s->ref[prev_frame].gm_params[ref][idx]; >>>> + >>>> if (idx < 2) { >>>> if (type == AV1_WARP_MODEL_TRANSLATION) { >>>> abs_bits = AV1_GM_ABS_TRANS_ONLY_BITS - >>>> @@ -131,7 +136,7 @@ static void read_global_param(AV1DecContext *s, >>>> int type, int ref, int idx) >>>> prec_diff = AV1_WARPEDMODEL_PREC_BITS - prec_bits; >>>> sub = (idx % 3) == 2 ? (1 << prec_bits) : 0; >>>> mx = 1 << abs_bits; >>>> - r = (s->ref[prev_frame].gm_params[ref][idx] >> prec_diff) - sub; >>>> + r = (prev_gm_param >> prec_diff) - sub; >>>> s->cur_frame.gm_params[ref][idx] = >>>> >>>> (decode_signed_subexp_with_ref(s->raw_frame_header->gm_params[ref][idx], >>>> >>>> > > LGTM with that. > > Thanks, Will apply, thanks. > > - Mark > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c index 54aeba1812..04aaf5d148 100644 --- a/libavcodec/av1dec.c +++ b/libavcodec/av1dec.c @@ -109,13 +109,18 @@ static void read_global_param(AV1DecContext *s, int type, int ref, int idx) { uint8_t primary_frame, prev_frame; uint32_t abs_bits, prec_bits, round, prec_diff, sub, mx; - int32_t r; + int32_t r, prev_gm_param; primary_frame = s->raw_frame_header->primary_ref_frame; prev_frame = s->raw_frame_header->ref_frame_idx[primary_frame]; abs_bits = AV1_GM_ABS_ALPHA_BITS; prec_bits = AV1_GM_ALPHA_PREC_BITS; + if (s->raw_frame_header->primary_ref_frame == AV1_PRIMARY_REF_NONE) + prev_gm_param = s->cur_frame.gm_params[ref][idx]; + else + prev_gm_param = s->ref[prev_frame].gm_params[ref][idx]; + if (idx < 2) { if (type == AV1_WARP_MODEL_TRANSLATION) { abs_bits = AV1_GM_ABS_TRANS_ONLY_BITS - @@ -131,7 +136,7 @@ static void read_global_param(AV1DecContext *s, int type, int ref, int idx) prec_diff = AV1_WARPEDMODEL_PREC_BITS - prec_bits; sub = (idx % 3) == 2 ? (1 << prec_bits) : 0; mx = 1 << abs_bits; - r = (s->ref[prev_frame].gm_params[ref][idx] >> prec_diff) - sub; + r = (prev_gm_param >> prec_diff) - sub; s->cur_frame.gm_params[ref][idx] = (decode_signed_subexp_with_ref(s->raw_frame_header->gm_params[ref][idx],
Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/av1dec.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)