[FFmpeg-devel,1/3] avcodec/cfhd: remove unused function

Submitted by Steven Liu on June 27, 2019, 7:38 a.m.

Details

Message ID 20190627073805.13322-1-lq@chinaffmpeg.org
State New
Headers show

Commit Message

Steven Liu June 27, 2019, 7:38 a.m.
Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
---
 libavcodec/cfhd.c | 14 --------------
 1 file changed, 14 deletions(-)

Comments

Paul B Mahol June 27, 2019, 8:36 a.m.
On 6/27/19, Steven Liu <lq@chinaffmpeg.org> wrote:
> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> ---
>  libavcodec/cfhd.c | 14 --------------
>  1 file changed, 14 deletions(-)
>
> diff --git a/libavcodec/cfhd.c b/libavcodec/cfhd.c
> index 846d334b9b..616f5af193 100644
> --- a/libavcodec/cfhd.c
> +++ b/libavcodec/cfhd.c
> @@ -136,20 +136,6 @@ static inline void peak_table(int16_t *band, Peak
> *peak, int length)
>              band[i] = bytestream2_get_le16(&peak->base);
>  }
>
> -static inline void process_alpha(int16_t *alpha, int width)
> -{
> -    int i, channel;
> -    for (i = 0; i < width; i++) {
> -        channel   = alpha[i];
> -        channel  -= ALPHA_COMPAND_DC_OFFSET;
> -        channel <<= 3;
> -        channel  *= ALPHA_COMPAND_GAIN;
> -        channel >>= 16;
> -        channel   = av_clip_uintp2(channel, 12);
> -        alpha[i]  = channel;
> -    }
> -}
> -
>  static inline void filter(int16_t *output, ptrdiff_t out_stride,
>                            int16_t *low, ptrdiff_t low_stride,
>                            int16_t *high, ptrdiff_t high_stride,
> --
> 2.17.2 (Apple Git-113)
>
>
>
> _______________________________________________
> 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".

NAK
Paul B Mahol June 27, 2019, 8:37 a.m.
On 6/27/19, Paul B Mahol <onemda@gmail.com> wrote:
> On 6/27/19, Steven Liu <lq@chinaffmpeg.org> wrote:
>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
>> ---
>>  libavcodec/cfhd.c | 14 --------------
>>  1 file changed, 14 deletions(-)
>>
>> diff --git a/libavcodec/cfhd.c b/libavcodec/cfhd.c
>> index 846d334b9b..616f5af193 100644
>> --- a/libavcodec/cfhd.c
>> +++ b/libavcodec/cfhd.c
>> @@ -136,20 +136,6 @@ static inline void peak_table(int16_t *band, Peak
>> *peak, int length)
>>              band[i] = bytestream2_get_le16(&peak->base);
>>  }
>>
>> -static inline void process_alpha(int16_t *alpha, int width)
>> -{
>> -    int i, channel;
>> -    for (i = 0; i < width; i++) {
>> -        channel   = alpha[i];
>> -        channel  -= ALPHA_COMPAND_DC_OFFSET;
>> -        channel <<= 3;
>> -        channel  *= ALPHA_COMPAND_GAIN;
>> -        channel >>= 16;
>> -        channel   = av_clip_uintp2(channel, 12);
>> -        alpha[i]  = channel;
>> -    }
>> -}
>> -
>>  static inline void filter(int16_t *output, ptrdiff_t out_stride,
>>                            int16_t *low, ptrdiff_t low_stride,
>>                            int16_t *high, ptrdiff_t high_stride,
>> --
>> 2.17.2 (Apple Git-113)
>>
>>
>>
>> _______________________________________________
>> 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".
>
> NAK
>

This code is needed for processing alpha, and should not be removed.
Steven Liu June 27, 2019, 8:53 a.m.
> 在 2019年6月27日,下午4:37,Paul B Mahol <onemda@gmail.com> 写道:
> 
> On 6/27/19, Paul B Mahol <onemda@gmail.com> wrote:
>> On 6/27/19, Steven Liu <lq@chinaffmpeg.org> wrote:
>>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
>>> ---
>>> libavcodec/cfhd.c | 14 --------------
>>> 1 file changed, 14 deletions(-)
>>> 
>>> diff --git a/libavcodec/cfhd.c b/libavcodec/cfhd.c
>>> index 846d334b9b..616f5af193 100644
>>> --- a/libavcodec/cfhd.c
>>> +++ b/libavcodec/cfhd.c
>>> @@ -136,20 +136,6 @@ static inline void peak_table(int16_t *band, Peak
>>> *peak, int length)
>>>             band[i] = bytestream2_get_le16(&peak->base);
>>> }
>>> 
>>> -static inline void process_alpha(int16_t *alpha, int width)
>>> -{
>>> -    int i, channel;
>>> -    for (i = 0; i < width; i++) {
>>> -        channel   = alpha[i];
>>> -        channel  -= ALPHA_COMPAND_DC_OFFSET;
>>> -        channel <<= 3;
>>> -        channel  *= ALPHA_COMPAND_GAIN;
>>> -        channel >>= 16;
>>> -        channel   = av_clip_uintp2(channel, 12);
>>> -        alpha[i]  = channel;
>>> -    }
>>> -}
>>> -
>>> static inline void filter(int16_t *output, ptrdiff_t out_stride,
>>>                           int16_t *low, ptrdiff_t low_stride,
>>>                           int16_t *high, ptrdiff_t high_stride,
>>> --
>>> 2.17.2 (Apple Git-113)
>>> 
>>> 
>>> 
>>> _______________________________________________
>>> 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".
>> 
>> NAK
>> 
> 
> This code is needed for processing alpha, and should not be removed.
I remove it because i cannot found the caller for this API.
I think there should have one caller for this API, It will not be used if there have no caller, Isn’t it?

> _______________________________________________
> 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".
Paul B Mahol June 27, 2019, 10:30 a.m.
On 6/27/19, Liu Steven <lq@chinaffmpeg.org> wrote:
>
>
>> 在 2019年6月27日,下午4:37,Paul B Mahol <onemda@gmail.com> 写道:
>>
>> On 6/27/19, Paul B Mahol <onemda@gmail.com> wrote:
>>> On 6/27/19, Steven Liu <lq@chinaffmpeg.org> wrote:
>>>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
>>>> ---
>>>> libavcodec/cfhd.c | 14 --------------
>>>> 1 file changed, 14 deletions(-)
>>>>
>>>> diff --git a/libavcodec/cfhd.c b/libavcodec/cfhd.c
>>>> index 846d334b9b..616f5af193 100644
>>>> --- a/libavcodec/cfhd.c
>>>> +++ b/libavcodec/cfhd.c
>>>> @@ -136,20 +136,6 @@ static inline void peak_table(int16_t *band, Peak
>>>> *peak, int length)
>>>>             band[i] = bytestream2_get_le16(&peak->base);
>>>> }
>>>>
>>>> -static inline void process_alpha(int16_t *alpha, int width)
>>>> -{
>>>> -    int i, channel;
>>>> -    for (i = 0; i < width; i++) {
>>>> -        channel   = alpha[i];
>>>> -        channel  -= ALPHA_COMPAND_DC_OFFSET;
>>>> -        channel <<= 3;
>>>> -        channel  *= ALPHA_COMPAND_GAIN;
>>>> -        channel >>= 16;
>>>> -        channel   = av_clip_uintp2(channel, 12);
>>>> -        alpha[i]  = channel;
>>>> -    }
>>>> -}
>>>> -
>>>> static inline void filter(int16_t *output, ptrdiff_t out_stride,
>>>>                           int16_t *low, ptrdiff_t low_stride,
>>>>                           int16_t *high, ptrdiff_t high_stride,
>>>> --
>>>> 2.17.2 (Apple Git-113)
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> 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".
>>>
>>> NAK
>>>
>>
>> This code is needed for processing alpha, and should not be removed.
> I remove it because i cannot found the caller for this API.
> I think there should have one caller for this API, It will not be used if
> there have no caller, Isn’t it?

This is not an API. Please leave this function alone. If you really
need to fix it,
look at prior commits to this file and re-add alpha support.

>
>> _______________________________________________
>> 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".
Nicolas George June 27, 2019, 10:33 a.m.
Liu Steven (12019-06-27):
> I remove it because i cannot found the caller for this API.
> I think there should have one caller for this API, It will not be used if there have no caller, Isn’t it?

I suppose you built the code and the compiler did not complain.

I see no reason to keep dead code in the source tree.

Regards,
Paul B Mahol June 27, 2019, 10:43 a.m.
On 6/27/19, Nicolas George <george@nsup.org> wrote:
> Liu Steven (12019-06-27):
>> I remove it because i cannot found the caller for this API.
>> I think there should have one caller for this API, It will not be used if
>> there have no caller, Isn’t it?
>
> I suppose you built the code and the compiler did not complain.
>
> I see no reason to keep dead code in the source tree.

No, code is not dead and should be kept, ask maintainer for confirmation.

>
> Regards,
>
> --
>   Nicolas George
>
Paul B Mahol June 27, 2019, 10:44 a.m.
On 6/27/19, Paul B Mahol <onemda@gmail.com> wrote:
> On 6/27/19, Nicolas George <george@nsup.org> wrote:
>> Liu Steven (12019-06-27):
>>> I remove it because i cannot found the caller for this API.
>>> I think there should have one caller for this API, It will not be used
>>> if
>>> there have no caller, Isn’t it?
>>
>> I suppose you built the code and the compiler did not complain.
>>
>> I see no reason to keep dead code in the source tree.
>
> No, code is not dead and should be kept, ask maintainer for confirmation.

Also, original author or this patch is actually Carl.

>
>>
>> Regards,
>>
>> --
>>   Nicolas George
>>
>
Nicolas George June 27, 2019, 10:54 a.m.
Paul B Mahol (12019-06-27):
> No, code is not dead and should be kept, ask maintainer for confirmation.

The compiler says it is. I am sorry, but your statement without any
proof seems less reliable than the diagnostics of the compiler and
Steven's judgement.

As is, the code can and should be removed, unless there is a technical
reason nobody mentioned yet.

Regards,
Andreas Rheinhardt June 27, 2019, 11:54 a.m.
Nicolas George:
> Paul B Mahol (12019-06-27):
>> No, code is not dead and should be kept, ask maintainer for confirmation.
> 
> The compiler says it is. I am sorry, but your statement without any
> proof seems less reliable than the diagnostics of the compiler and
> Steven's judgement.
> 
> As is, the code can and should be removed, unless there is a technical
> reason nobody mentioned yet.
> 
> Regards,
The code is indeed dead atm. To quote myself from ticket 7886:
"Commit c64c97b972c7325a71440a352a7d541a8c92b2da has added support for
alpha channel decoding in Cineform HD (thereby fixing #6265), but
commit 9cefb9e7ec508900ba147e6977590f03456aa15c broke it again (the
function process_alpha introduced in the first commit is now not even
used any more -- thanks to Clang for reporting this). The sample
cineform_rgba_12b.mov from #6265 now decodes as before
c64c97b972c7325a71440a352a7d541a8c92b2da."

- Andreas
Nicolas George June 27, 2019, 12:01 p.m.
Andreas Rheinhardt (12019-06-27):
> The code is indeed dead atm. To quote myself from ticket 7886:
> "Commit c64c97b972c7325a71440a352a7d541a8c92b2da has added support for
> alpha channel decoding in Cineform HD (thereby fixing #6265), but
> commit 9cefb9e7ec508900ba147e6977590f03456aa15c broke it again (the
> function process_alpha introduced in the first commit is now not even
> used any more -- thanks to Clang for reporting this). The sample
> cineform_rgba_12b.mov from #6265 now decodes as before
> c64c97b972c7325a71440a352a7d541a8c92b2da."

Thanks for clarifying. If it is dead, it can be removed. If it is needed
back, then it can be added back. Steven, go ahead if you want.

Regards,
Kieran Kunhya June 27, 2019, 12:36 p.m.
On Thu, 27 Jun 2019 at 20:01, Nicolas George <george@nsup.org> wrote:

> Andreas Rheinhardt (12019-06-27):
> > The code is indeed dead atm. To quote myself from ticket 7886:
> > "Commit c64c97b972c7325a71440a352a7d541a8c92b2da has added support for
> > alpha channel decoding in Cineform HD (thereby fixing #6265), but
> > commit 9cefb9e7ec508900ba147e6977590f03456aa15c broke it again (the
> > function process_alpha introduced in the first commit is now not even
> > used any more -- thanks to Clang for reporting this). The sample
> > cineform_rgba_12b.mov from #6265 now decodes as before
> > c64c97b972c7325a71440a352a7d541a8c92b2da."
>
> Thanks for clarifying. If it is dead, it can be removed. If it is needed
> back, then it can be added back. Steven, go ahead if you want.
>

Why can this not be fixed properly? If the call to the function has been
removed accidentally, why make things worse and remove the function
completely?
This makes no sense at all.

Kieran
Nicolas George June 27, 2019, 12:52 p.m.
Kieran Kunhya (12019-06-27):
> Why can this not be fixed properly?

It can. If you have time and motivation to do it, please go ahead.
Barring that, removing dead code is still an improvement.

Regards,
Kieran Kunhya June 27, 2019, 1:06 p.m.
On Thu, 27 Jun 2019 at 20:52, Nicolas George <george@nsup.org> wrote:

> Kieran Kunhya (12019-06-27):
> > Why can this not be fixed properly?
>
> It can. If you have time and motivation to do it, please go ahead.
> Barring that, removing dead code is still an improvement.
>
> Regards,
>

It's not dead code, the GSOC student last year removed the call to the
function by accident.
It is beyond comprehension how removing more code and making the situation
worse improves things.

Kieran
Nicolas George June 27, 2019, 1:29 p.m.
Kieran Kunhya (12019-06-27):
> It's not dead code,

Right now, yes, it is dead.

> the GSOC student last year removed the call to the
> function by accident.

If it is so easy, you could do it instead of arguing. If it is not so
easy, you cannot demand somebody do it.

> It is beyond comprehension how removing more code and making the situation
> worse improves things.

Dead code is never good.

Regards,
Kieran Kunhya June 27, 2019, 1:43 p.m.
>
> If it is so easy, you could do it instead of arguing. If it is not so
> easy, you cannot demand somebody do it.
>

I'm happy to do it now that I am aware of the issue. I will do it when I am
at home in a few days.


> > It is beyond comprehension how removing more code and making the
> situation
> > worse improves things.
>
> Dead code is never good.
>

This absolutism is absurd.

Kieran
Nicolas George June 27, 2019, 1:44 p.m.
Kieran Kunhya (12019-06-27):
> I'm happy to do it now that I am aware of the issue. I will do it when I am
> at home in a few days.

Thanks. I am sure Steven will not mind waiting a few days.

> This absolutism is absurd.

Do you have an example of situation where dead code is good?

Regards,
James Almer June 27, 2019, 1:52 p.m.
On 6/27/2019 10:44 AM, Nicolas George wrote:
> Kieran Kunhya (12019-06-27):
>> I'm happy to do it now that I am aware of the issue. I will do it when I am
>> at home in a few days.
> 
> Thanks. I am sure Steven will not mind waiting a few days.
> 
>> This absolutism is absurd.
> 
> Do you have an example of situation where dead code is good?

At least for this specific case, not littering the git history with
commits removing something that's going to be readded in a couple days
when a proper fix to a known bug is implemented instead, and not ruining
the usefulness of git blame for debug/bisect/backtracking purposes.
Vittorio Giovara June 27, 2019, 3:35 p.m.
On Thu, Jun 27, 2019 at 9:44 AM Nicolas George <george@nsup.org> wrote:

> Kieran Kunhya (12019-06-27):
> > I'm happy to do it now that I am aware of the issue. I will do it when I
> am
> > at home in a few days.
>
> Thanks. I am sure Steven will not mind waiting a few days.
>
> > This absolutism is absurd.
>
> Do you have an example of situation where dead code is good?
>

If i could add my 2 cents, for a reverse engineered codec it's important to
leave unused functions purely for documentation purposes, so that future
maintainers could implement and read about it right away, rather than
digging in a large and messy git history.
Additionally most compilers run passes that drop dead code already in a way
that does not affect runtime one bit. So I really don't see any gains in
removing these 14 lines of code.
Reimar Döffinger June 27, 2019, 8:25 p.m.
On 27.06.2019, at 17:35, Vittorio Giovara <vittorio.giovara@gmail.com> wrote:

> On Thu, Jun 27, 2019 at 9:44 AM Nicolas George <george@nsup.org> wrote:
> 
>> Kieran Kunhya (12019-06-27):
>>> I'm happy to do it now that I am aware of the issue. I will do it when I
>> am
>>> at home in a few days.
>> 
>> Thanks. I am sure Steven will not mind waiting a few days.
>> 
>>> This absolutism is absurd.
>> 
>> Do you have an example of situation where dead code is good?
>> 
> 
> If i could add my 2 cents, for a reverse engineered codec it's important to
> leave unused functions purely for documentation purposes, so that future
> maintainers could implement and read about it right away, rather than
> digging in a large and messy git history.
> Additionally most compilers run passes that drop dead code already in a way
> that does not affect runtime one bit. So I really don't see any gains in
> removing these 14 lines of code.

I'd note that intentionally dead code should at least have a comment, and
maybe even a #if 0 would make sense (though has the disadvantage of not
even compile-testing the code).
In case of an actual bug like here I would say dead code if nothing else is a reminder of the
bug, though admittedly a very poor one.
Either way I suggest to discuss such things more relaxed, a few days more or
less hardly matters and there might be useful insights from other people (of
course I don't mean to delay non-controversial stuff nobody has any comments/objections on).

Best regards,
Reimar Döffinger
Steven Liu June 27, 2019, 11:06 p.m.
> 在 2019年6月28日,04:25,Reimar Döffinger <Reimar.Doeffinger@gmx.de> 写道:
> 
> On 27.06.2019, at 17:35, Vittorio Giovara <vittorio.giovara@gmail.com> wrote:
> 
>> On Thu, Jun 27, 2019 at 9:44 AM Nicolas George <george@nsup.org> wrote:
>> 
>>> Kieran Kunhya (12019-06-27):
>>>> I'm happy to do it now that I am aware of the issue. I will do it when I
>>> am
>>>> at home in a few days.
>>> 
>>> Thanks. I am sure Steven will not mind waiting a few days.
>>> 
>>>> This absolutism is absurd.
>>> 
>>> Do you have an example of situation where dead code is good?
>>> 
>> 
>> If i could add my 2 cents, for a reverse engineered codec it's important to
>> leave unused functions purely for documentation purposes, so that future
>> maintainers could implement and read about it right away, rather than
>> digging in a large and messy git history.
>> Additionally most compilers run passes that drop dead code already in a way
>> that does not affect runtime one bit. So I really don't see any gains in
>> removing these 14 lines of code.
> 
> I'd note that intentionally dead code should at least have a comment, and
> maybe even a #if 0
Yes, i think if the maintainers want use the unused code shortly time, maybe #if 0 is better way to fix the compiling waring.
Because that is means the code is not in use now, perhaps it will be used In the future, so should add comment for that.
not just stay it here, compiler give me warning, that is true, it’s dirty when compiling the project.
If there have much more warning when compiling, i think no people like full screen warning message.
perhaps there will have second, three, four, and so on if ignore the first one with this kind of message.
Make the code clean, At the very least, no warning when compiling.

> would make sense (though has the disadvantage of not
> even compile-testing the code).
> In case of an actual bug like here I would say dead code if nothing else is a reminder of the
> bug, though admittedly a very poor one.1
> Either way I suggest to discuss such things more relaxed, a few days more or
> less hardly matters and there might be useful insights from other people (of
> course I don't mean to delay non-controversial stuff nobody has any comments/objections on).
> 
> Best regards,
> Reimar Döffinger
> _______________________________________________
> 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
Steven

Patch hide | download patch | download mbox

diff --git a/libavcodec/cfhd.c b/libavcodec/cfhd.c
index 846d334b9b..616f5af193 100644
--- a/libavcodec/cfhd.c
+++ b/libavcodec/cfhd.c
@@ -136,20 +136,6 @@  static inline void peak_table(int16_t *band, Peak *peak, int length)
             band[i] = bytestream2_get_le16(&peak->base);
 }
 
-static inline void process_alpha(int16_t *alpha, int width)
-{
-    int i, channel;
-    for (i = 0; i < width; i++) {
-        channel   = alpha[i];
-        channel  -= ALPHA_COMPAND_DC_OFFSET;
-        channel <<= 3;
-        channel  *= ALPHA_COMPAND_GAIN;
-        channel >>= 16;
-        channel   = av_clip_uintp2(channel, 12);
-        alpha[i]  = channel;
-    }
-}
-
 static inline void filter(int16_t *output, ptrdiff_t out_stride,
                           int16_t *low, ptrdiff_t low_stride,
                           int16_t *high, ptrdiff_t high_stride,