| Message ID | 20190627073805.13322-1-lq@chinaffmpeg.org |
|---|---|
| State | New |
| Headers | show |
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
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.
> 在 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".
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".
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,
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 >
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 >> >
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,
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
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,
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
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,
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
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,
> > 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
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,
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.
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.
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
> 在 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
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,
Signed-off-by: Steven Liu <lq@chinaffmpeg.org> --- libavcodec/cfhd.c | 14 -------------- 1 file changed, 14 deletions(-)