| Message ID | 20210617061037.20335-1-fei.w.wang@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel,1/9] cbs_av1: fix incorrect data type | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
On Thu, 2021-06-17 at 14:10 +0800, Fei Wang wrote: > shifted_order_hints is computed by data with int plus data with int. > Switch to int8_t may lose its precision. > > Signed-off-by: Fei Wang <fei.w.wang@intel.com> > --- > libavcodec/cbs_av1_syntax_template.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/cbs_av1_syntax_template.c > b/libavcodec/cbs_av1_syntax_template.c > index 6fe6e9a4f3..956d45e132 100644 > --- a/libavcodec/cbs_av1_syntax_template.c > +++ b/libavcodec/cbs_av1_syntax_template.c > @@ -355,7 +355,7 @@ static int > FUNC(set_frame_refs)(CodedBitstreamContext *ctx, RWContext *rw, > AV1_REF_FRAME_ALTREF2, AV1_REF_FRAME_ALTREF > }; > int8_t ref_frame_idx[AV1_REFS_PER_FRAME], > used_frame[AV1_NUM_REF_FRAMES]; > - int8_t shifted_order_hints[AV1_NUM_REF_FRAMES]; > + int shifted_order_hints[AV1_NUM_REF_FRAMES]; Hi, Ping for review this patch set. Thanks. Fei > int cur_frame_hint, latest_order_hint, earliest_order_hint, ref; > int i, j; >
On Mon, 2021-06-21 at 09:38 +0800, Fei.Wang wrote: > On Thu, 2021-06-17 at 14:10 +0800, Fei Wang wrote: > > shifted_order_hints is computed by data with int plus data with > > int. > > Switch to int8_t may lose its precision. > > > > Signed-off-by: Fei Wang <fei.w.wang@intel.com> > > --- > > libavcodec/cbs_av1_syntax_template.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/libavcodec/cbs_av1_syntax_template.c > > b/libavcodec/cbs_av1_syntax_template.c > > index 6fe6e9a4f3..956d45e132 100644 > > --- a/libavcodec/cbs_av1_syntax_template.c > > +++ b/libavcodec/cbs_av1_syntax_template.c > > @@ -355,7 +355,7 @@ static int > > FUNC(set_frame_refs)(CodedBitstreamContext *ctx, RWContext *rw, > > AV1_REF_FRAME_ALTREF2, AV1_REF_FRAME_ALTREF > > }; > > int8_t ref_frame_idx[AV1_REFS_PER_FRAME], > > used_frame[AV1_NUM_REF_FRAMES]; > > - int8_t shifted_order_hints[AV1_NUM_REF_FRAMES]; > > + int shifted_order_hints[AV1_NUM_REF_FRAMES]; > > Hi, > Ping for review this patch set. Thanks. Hi James, Free to help review this patch set? This change improved VAAPI av1 decoder quality quite a lot base on our test result, and I believe it can benefit to all other HW av1 decoders too. Thanks > > Fei > > > int cur_frame_hint, latest_order_hint, earliest_order_hint, > > ref; > > int i, j; > >
On 6/17/2021 3:10 AM, Fei Wang wrote: > shifted_order_hints is computed by data with int plus data with int. > Switch to int8_t may lose its precision. > > Signed-off-by: Fei Wang <fei.w.wang@intel.com> > --- > libavcodec/cbs_av1_syntax_template.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/cbs_av1_syntax_template.c b/libavcodec/cbs_av1_syntax_template.c > index 6fe6e9a4f3..956d45e132 100644 > --- a/libavcodec/cbs_av1_syntax_template.c > +++ b/libavcodec/cbs_av1_syntax_template.c > @@ -355,7 +355,7 @@ static int FUNC(set_frame_refs)(CodedBitstreamContext *ctx, RWContext *rw, > AV1_REF_FRAME_ALTREF2, AV1_REF_FRAME_ALTREF > }; > int8_t ref_frame_idx[AV1_REFS_PER_FRAME], used_frame[AV1_NUM_REF_FRAMES]; > - int8_t shifted_order_hints[AV1_NUM_REF_FRAMES]; > + int shifted_order_hints[AV1_NUM_REF_FRAMES]; Would int16_t be enough? If so, use that. LGTM either way. > int cur_frame_hint, latest_order_hint, earliest_order_hint, ref; > int i, j; > >
On Thu, 2021-07-01 at 09:41 -0300, James Almer wrote: > On 6/17/2021 3:10 AM, Fei Wang wrote: > > shifted_order_hints is computed by data with int plus data with > > int. > > Switch to int8_t may lose its precision. > > > > Signed-off-by: Fei Wang <fei.w.wang@intel.com> > > --- > > libavcodec/cbs_av1_syntax_template.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/libavcodec/cbs_av1_syntax_template.c > > b/libavcodec/cbs_av1_syntax_template.c > > index 6fe6e9a4f3..956d45e132 100644 > > --- a/libavcodec/cbs_av1_syntax_template.c > > +++ b/libavcodec/cbs_av1_syntax_template.c > > @@ -355,7 +355,7 @@ static int > > FUNC(set_frame_refs)(CodedBitstreamContext *ctx, RWContext *rw, > > AV1_REF_FRAME_ALTREF2, AV1_REF_FRAME_ALTREF > > }; > > int8_t ref_frame_idx[AV1_REFS_PER_FRAME], > > used_frame[AV1_NUM_REF_FRAMES]; > > - int8_t shifted_order_hints[AV1_NUM_REF_FRAMES]; > > + int shifted_order_hints[AV1_NUM_REF_FRAMES]; > > Would int16_t be enough? If so, use that. int16_t can fixed my clip. But as I mentioned in commit message, this variable is get with int plus int, switch to int16_t may still has potential threat. Also when shifted_order_hints is called, it turns back to int again: int hint = shifted_order_hints[i]; Thanks Fei > > LGTM either way. > > > int cur_frame_hint, latest_order_hint, earliest_order_hint, > > ref; > > int i, j; > > > > > > _______________________________________________ > 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 7/2/2021 2:55 AM, Wang, Fei W wrote: > On Thu, 2021-07-01 at 09:41 -0300, James Almer wrote: >> On 6/17/2021 3:10 AM, Fei Wang wrote: >>> shifted_order_hints is computed by data with int plus data with >>> int. >>> Switch to int8_t may lose its precision. >>> >>> Signed-off-by: Fei Wang <fei.w.wang@intel.com> >>> --- >>> libavcodec/cbs_av1_syntax_template.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/libavcodec/cbs_av1_syntax_template.c >>> b/libavcodec/cbs_av1_syntax_template.c >>> index 6fe6e9a4f3..956d45e132 100644 >>> --- a/libavcodec/cbs_av1_syntax_template.c >>> +++ b/libavcodec/cbs_av1_syntax_template.c >>> @@ -355,7 +355,7 @@ static int >>> FUNC(set_frame_refs)(CodedBitstreamContext *ctx, RWContext *rw, >>> AV1_REF_FRAME_ALTREF2, AV1_REF_FRAME_ALTREF >>> }; >>> int8_t ref_frame_idx[AV1_REFS_PER_FRAME], >>> used_frame[AV1_NUM_REF_FRAMES]; >>> - int8_t shifted_order_hints[AV1_NUM_REF_FRAMES]; >>> + int shifted_order_hints[AV1_NUM_REF_FRAMES]; >> >> Would int16_t be enough? If so, use that. > > int16_t can fixed my clip. But as I mentioned in commit message, this > variable is get with int plus int, switch to int16_t may still has > potential threat. It wont since seq->order_hint_bits_minus_1 has a range of 0-7, meaning cur_frame_hint can be at most 128. Similar situation for the return value of cbs_av1_get_relative_dist(). > > Also when shifted_order_hints is called, it turns back to int again: > > int hint = shifted_order_hints[i]; For arrays we prefer fixed size types, whereas for scalars native sizes are better. > > Thanks > Fei > >> >> LGTM either way. >> >>> int cur_frame_hint, latest_order_hint, earliest_order_hint, >>> ref; >>> int i, j; >>> >>> >> >> _______________________________________________ >> 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". > _______________________________________________ > 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 Fri, 2021-07-02 at 08:29 -0300, James Almer wrote: > On 7/2/2021 2:55 AM, Wang, Fei W wrote: > > On Thu, 2021-07-01 at 09:41 -0300, James Almer wrote: > > > On 6/17/2021 3:10 AM, Fei Wang wrote: > > > > shifted_order_hints is computed by data with int plus data with > > > > int. > > > > Switch to int8_t may lose its precision. > > > > > > > > Signed-off-by: Fei Wang <fei.w.wang@intel.com> > > > > --- > > > > libavcodec/cbs_av1_syntax_template.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/libavcodec/cbs_av1_syntax_template.c > > > > b/libavcodec/cbs_av1_syntax_template.c > > > > index 6fe6e9a4f3..956d45e132 100644 > > > > --- a/libavcodec/cbs_av1_syntax_template.c > > > > +++ b/libavcodec/cbs_av1_syntax_template.c > > > > @@ -355,7 +355,7 @@ static int > > > > FUNC(set_frame_refs)(CodedBitstreamContext *ctx, RWContext *rw, > > > > AV1_REF_FRAME_ALTREF2, AV1_REF_FRAME_ALTREF > > > > }; > > > > int8_t ref_frame_idx[AV1_REFS_PER_FRAME], > > > > used_frame[AV1_NUM_REF_FRAMES]; > > > > - int8_t shifted_order_hints[AV1_NUM_REF_FRAMES]; > > > > + int shifted_order_hints[AV1_NUM_REF_FRAMES]; > > > > > > Would int16_t be enough? If so, use that. > > > > int16_t can fixed my clip. But as I mentioned in commit message, > > this > > variable is get with int plus int, switch to int16_t may still has > > potential threat. > > It wont since seq->order_hint_bits_minus_1 has a range of 0-7, > meaning > cur_frame_hint can be at most 128. Similar situation for the return > value of cbs_av1_get_relative_dist(). That's make sense. Thanks James. I will submit V2 to use int16_t. Thanks Fei > > > > > Also when shifted_order_hints is called, it turns back to int > > again: > > > > int hint = shifted_order_hints[i]; > > For arrays we prefer fixed size types, whereas for scalars native > sizes > are better. > > > > > Thanks > > Fei > > > > > > > > LGTM either way. > > > > > > > int cur_frame_hint, latest_order_hint, > > > > earliest_order_hint, > > > > ref; > > > > int i, j; > > > > > > > > > > > > > > _______________________________________________ > > > 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". > > > > _______________________________________________ > > 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". > > > > _______________________________________________ > 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/cbs_av1_syntax_template.c b/libavcodec/cbs_av1_syntax_template.c index 6fe6e9a4f3..956d45e132 100644 --- a/libavcodec/cbs_av1_syntax_template.c +++ b/libavcodec/cbs_av1_syntax_template.c @@ -355,7 +355,7 @@ static int FUNC(set_frame_refs)(CodedBitstreamContext *ctx, RWContext *rw, AV1_REF_FRAME_ALTREF2, AV1_REF_FRAME_ALTREF }; int8_t ref_frame_idx[AV1_REFS_PER_FRAME], used_frame[AV1_NUM_REF_FRAMES]; - int8_t shifted_order_hints[AV1_NUM_REF_FRAMES]; + int shifted_order_hints[AV1_NUM_REF_FRAMES]; int cur_frame_hint, latest_order_hint, earliest_order_hint, ref; int i, j;
shifted_order_hints is computed by data with int plus data with int. Switch to int8_t may lose its precision. Signed-off-by: Fei Wang <fei.w.wang@intel.com> --- libavcodec/cbs_av1_syntax_template.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)