diff mbox series

[FFmpeg-devel,1/9] cbs_av1: fix incorrect data type

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
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished

Commit Message

Wang, Fei W June 17, 2021, 6:10 a.m. UTC
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(-)

Comments

Wang, Fei W June 21, 2021, 1:40 a.m. UTC | #1
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;
>
Wang, Fei W July 1, 2021, 8:21 a.m. UTC | #2
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;
> >
James Almer July 1, 2021, 12:41 p.m. UTC | #3
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;
>   
>
Wang, Fei W July 2, 2021, 5:55 a.m. UTC | #4
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".
James Almer July 2, 2021, 11:29 a.m. UTC | #5
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".
>
Wang, Fei W July 5, 2021, 1:32 a.m. UTC | #6
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 mbox series

Patch

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;