[FFmpeg-devel] [PATCH] avcodec/aacenc_tns: remove unreachable code branch

[FFmpeg-devel] [PATCH] avcodec/aacenc_tns: remove unreachable code branch

[Yotam Ofek]

the condition being tested was the same as the stop condition for the containing loop,
so inside the loop it would always test positive
---
 libavcodec/aacenc_tns.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/libavcodec/aacenc_tns.c b/libavcodec/aacenc_tns.c
index 60888fece7..fa3cd2af39 100644
--- a/libavcodec/aacenc_tns.c
+++ b/libavcodec/aacenc_tns.c
@@ -181,7 +181,7 @@ void ff_aac_search_for_tns(AACEncContext *s, SingleChannelElement *sce)
 
     for (w = 0; w < sce->ics.num_windows; w++) {
         float en[2] = {0.0f, 0.0f};
-        int oc_start = 0, os_start = 0;
+        int oc_start = 0;
         int coef_start = sce->ics.swb_offset[sfb_start];
 
         for (g = sfb_start; g < sce->ics.num_swb && g <= sfb_end; g++) {
@@ -202,12 +202,11 @@ void ff_aac_search_for_tns(AACEncContext *s, SingleChannelElement *sce)
         tns->n_filt[w] = is8 ? 1 : order != TNS_MAX_ORDER ? 2 : 3;
         for (g = 0; g < tns->n_filt[w]; g++) {
             tns->direction[w][g] = slant != 2 ? slant : en[g] < en[!g];
-            tns->order[w][g] = g < tns->n_filt[w] ? order/tns->n_filt[w] : order - oc_start;
-            tns->length[w][g] = g < tns->n_filt[w] ? sfb_len/tns->n_filt[w] : sfb_len - os_start;
+            tns->order[w][g] = order/tns->n_filt[w];
+            tns->length[w][g] = sfb_len/tns->n_filt[w];
             quantize_coefs(&coefs[oc_start], tns->coef_idx[w][g], tns->coef[w][g],
                             tns->order[w][g], c_bits);
             oc_start += tns->order[w][g];
-            os_start += tns->length[w][g];
         }
         count++;
     }
-- 
2.40.1

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/aacenc_tns: remove unreachable code branch

[Timo Rothenpieler]

On 12.05.2024 10:53, Yotam Ofek wrote:
> the condition being tested was the same as the stop condition for the containing loop,
> so inside the loop it would always test positive
> ---
>   libavcodec/aacenc_tns.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/libavcodec/aacenc_tns.c b/libavcodec/aacenc_tns.c
> index 60888fece7..fa3cd2af39 100644
> --- a/libavcodec/aacenc_tns.c
> +++ b/libavcodec/aacenc_tns.c
> @@ -181,7 +181,7 @@ void ff_aac_search_for_tns(AACEncContext *s, SingleChannelElement *sce)
>   
>       for (w = 0; w < sce->ics.num_windows; w++) {
>           float en[2] = {0.0f, 0.0f};
> -        int oc_start = 0, os_start = 0;
> +        int oc_start = 0;
>           int coef_start = sce->ics.swb_offset[sfb_start];
>   
>           for (g = sfb_start; g < sce->ics.num_swb && g <= sfb_end; g++) {
> @@ -202,12 +202,11 @@ void ff_aac_search_for_tns(AACEncContext *s, SingleChannelElement *sce)
>           tns->n_filt[w] = is8 ? 1 : order != TNS_MAX_ORDER ? 2 : 3;
>           for (g = 0; g < tns->n_filt[w]; g++) {
>               tns->direction[w][g] = slant != 2 ? slant : en[g] < en[!g];
> -            tns->order[w][g] = g < tns->n_filt[w] ? order/tns->n_filt[w] : order - oc_start;
> -            tns->length[w][g] = g < tns->n_filt[w] ? sfb_len/tns->n_filt[w] : sfb_len - os_start;
> +            tns->order[w][g] = order/tns->n_filt[w];
> +            tns->length[w][g] = sfb_len/tns->n_filt[w];

Are you sure this isn't an indication that this code was meant to do 
something else, and there is actually a different issue than just dead 
code here?

>               quantize_coefs(&coefs[oc_start], tns->coef_idx[w][g], tns->coef[w][g],
>                               tns->order[w][g], c_bits);
>               oc_start += tns->order[w][g];
> -            os_start += tns->length[w][g];
>           }
>           count++;
>       }
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/aacenc_tns: remove unreachable code branch

[Yotam Ofek]

Nope :)
Last commit that touched this was 9 years ago, seems like it's an oversight
that was never noticed/fixed.
The native AAC encoder is stable, even if this code was supposed to behave
differently, it's probably good enough as is, and I think that having this
redundant conditional is bad for readability and maintenance (definitely
confused me...)


On Sun, May 12, 2024 at 2:19 PM Timo Rothenpieler <timo@rothenpieler.org>
wrote:

> On 12.05.2024 10:53, Yotam Ofek wrote:
> > the condition being tested was the same as the stop condition for the
> containing loop,
> > so inside the loop it would always test positive
> > ---
> >   libavcodec/aacenc_tns.c | 7 +++----
> >   1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/libavcodec/aacenc_tns.c b/libavcodec/aacenc_tns.c
> > index 60888fece7..fa3cd2af39 100644
> > --- a/libavcodec/aacenc_tns.c
> > +++ b/libavcodec/aacenc_tns.c
> > @@ -181,7 +181,7 @@ void ff_aac_search_for_tns(AACEncContext *s,
> SingleChannelElement *sce)
> >
> >       for (w = 0; w < sce->ics.num_windows; w++) {
> >           float en[2] = {0.0f, 0.0f};
> > -        int oc_start = 0, os_start = 0;
> > +        int oc_start = 0;
> >           int coef_start = sce->ics.swb_offset[sfb_start];
> >
> >           for (g = sfb_start; g < sce->ics.num_swb && g <= sfb_end; g++)
> {
> > @@ -202,12 +202,11 @@ void ff_aac_search_for_tns(AACEncContext *s,
> SingleChannelElement *sce)
> >           tns->n_filt[w] = is8 ? 1 : order != TNS_MAX_ORDER ? 2 : 3;
> >           for (g = 0; g < tns->n_filt[w]; g++) {
> >               tns->direction[w][g] = slant != 2 ? slant : en[g] < en[!g];
> > -            tns->order[w][g] = g < tns->n_filt[w] ?
> order/tns->n_filt[w] : order - oc_start;
> > -            tns->length[w][g] = g < tns->n_filt[w] ?
> sfb_len/tns->n_filt[w] : sfb_len - os_start;
> > +            tns->order[w][g] = order/tns->n_filt[w];
> > +            tns->length[w][g] = sfb_len/tns->n_filt[w];
>
> Are you sure this isn't an indication that this code was meant to do
> something else, and there is actually a different issue than just dead
> code here?
>
> >               quantize_coefs(&coefs[oc_start], tns->coef_idx[w][g],
> tns->coef[w][g],
> >                               tns->order[w][g], c_bits);
> >               oc_start += tns->order[w][g];
> > -            os_start += tns->length[w][g];
> >           }
> >           count++;
> >       }
> _______________________________________________
> 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".

Re: [FFmpeg-devel] [PATCH] avcodec/aacenc_tns: remove unreachable code branch

[Lynne via ffmpeg-devel]

On 12/05/2024 14:24, Yotam Ofek wrote:
> Nope :)
> Last commit that touched this was 9 years ago, seems like it's an oversight
> that was never noticed/fixed.
> The native AAC encoder is stable, even if this code was supposed to behave
> differently, it's probably good enough as is, and I think that having this
> redundant conditional is bad for readability and maintenance (definitely
> confused me...)
> 
> 
> On Sun, May 12, 2024 at 2:19 PM Timo Rothenpieler <timo@rothenpieler.org>
> wrote:
> 
>> On 12.05.2024 10:53, Yotam Ofek wrote:
>>> the condition being tested was the same as the stop condition for the
>> containing loop,
>>> so inside the loop it would always test positive
>>> ---
>>>    libavcodec/aacenc_tns.c | 7 +++----
>>>    1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/libavcodec/aacenc_tns.c b/libavcodec/aacenc_tns.c
>>> index 60888fece7..fa3cd2af39 100644
>>> --- a/libavcodec/aacenc_tns.c
>>> +++ b/libavcodec/aacenc_tns.c
>>> @@ -181,7 +181,7 @@ void ff_aac_search_for_tns(AACEncContext *s,
>> SingleChannelElement *sce)
>>>
>>>        for (w = 0; w < sce->ics.num_windows; w++) {
>>>            float en[2] = {0.0f, 0.0f};
>>> -        int oc_start = 0, os_start = 0;
>>> +        int oc_start = 0;
>>>            int coef_start = sce->ics.swb_offset[sfb_start];
>>>
>>>            for (g = sfb_start; g < sce->ics.num_swb && g <= sfb_end; g++)
>> {
>>> @@ -202,12 +202,11 @@ void ff_aac_search_for_tns(AACEncContext *s,
>> SingleChannelElement *sce)
>>>            tns->n_filt[w] = is8 ? 1 : order != TNS_MAX_ORDER ? 2 : 3;
>>>            for (g = 0; g < tns->n_filt[w]; g++) {
>>>                tns->direction[w][g] = slant != 2 ? slant : en[g] < en[!g];
>>> -            tns->order[w][g] = g < tns->n_filt[w] ?
>> order/tns->n_filt[w] : order - oc_start;
>>> -            tns->length[w][g] = g < tns->n_filt[w] ?
>> sfb_len/tns->n_filt[w] : sfb_len - os_start;
>>> +            tns->order[w][g] = order/tns->n_filt[w];
>>> +            tns->length[w][g] = sfb_len/tns->n_filt[w];
>>
>> Are you sure this isn't an indication that this code was meant to do
>> something else, and there is actually a different issue than just dead
>> code here?
>>
>>>                quantize_coefs(&coefs[oc_start], tns->coef_idx[w][g],
>> tns->coef[w][g],
>>>                                tns->order[w][g], c_bits);
>>>                oc_start += tns->order[w][g];
>>> -            os_start += tns->length[w][g];
>>>            }
>>>            count++;
>>>        }
>> _______________________________________________
>> 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".

Thanks, pushed.
I have no idea what this was meant to do, but whatever it is, it's not 
correct. To begin with, you barely have 4 bits for the length and 6/4 
for the order, much less than the #SFBs. USAC even reduced the order 
down to 5/3 bits.
The code should do proper RDO here using the dequantized coeffs to 
figure out if TNS can help, as TNS settings themselves consume around 60 
bits in total for general inputs and only make sense when you have 
actual coefficients in a band after dequant, rather than a quantized 
mess full of spectral holes.
_______________________________________________
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".