[FFmpeg-devel,1/4] lavc/dnxhdenc: eliminate dead code

dct_quantize cannot be NULL here. The call to ff_dct_encode_init() would
initialized it to a non-NULL value already.
---
 libavcodec/dnxhdenc.c | 3 ---
 1 file changed, 3 deletions(-)

Rémi Denis-Courmont:
> On entry the function pointer is always NULL. We just need to set the
> pointer before probing x86 CPU optimisations.
> 

Incorrect:
https://github.com/mkver/FFmpeg/commit/d22d4ee8419788f9bb239a21e276cebce0891737
(see also
https://github.com/mkver/FFmpeg/commits/mpegvideo_pool/?after=d2dfcf8f226c3708f3df080aed043ff4aa26e7cd+34
which contains the equivalent of patches 1+2 and a better version of #4)

> Note that there is a third code path setting this function pointer, but
> it does so *after* calling this function: the DNxHD encoder.
> ---
>  libavcodec/mpegvideo_enc.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/libavcodec/mpegvideo_enc.c b/libavcodec/mpegvideo_enc.c
> index af04db70d8..49eed6301b 100644
> --- a/libavcodec/mpegvideo_enc.c
> +++ b/libavcodec/mpegvideo_enc.c
> @@ -371,14 +371,13 @@ static void mpv_encode_defaults(MpegEncContext *s)
>  
>  av_cold int ff_dct_encode_init(MpegEncContext *s)
>  {
> +    s->dct_quantize = dct_quantize_c;
>  #if ARCH_X86
>      ff_dct_encode_init_x86(s);
>  #endif
>  
>      if (CONFIG_H263_ENCODER)
>          ff_h263dsp_init(&s->h263dsp);
> -    if (!s->dct_quantize)
> -        s->dct_quantize = dct_quantize_c;
>      if (!s->denoise_dct)
>          s->denoise_dct  = denoise_dct_c;
>      s->fast_dct_quantize = s->dct_quantize;

Le sunnuntaina 9. kesäkuuta 2024, 11.39.55 EEST Andreas Rheinhardt a écrit :
> Andreas Rheinhardt:
> > Rémi Denis-Courmont:
> >> On entry the function pointer is always NULL. We just need to set the
> >> pointer before probing x86 CPU optimisations.
> > 
> > Incorrect:
> > https://github.com/mkver/FFmpeg/commit/d22d4ee8419788f9bb239a21e276cebce08
> > 91737 (see also
> > https://github.com/mkver/FFmpeg/commits/mpegvideo_pool/?after=d2dfcf8f226c
> > 3708f3df080aed043ff4aa26e7cd+34 which contains the equivalent of patches
> > 1+2 and a better version of #4)
> Wait, I see that you only set dct_quantize unconditionally. So your
> claim that dct_quantize is always NULL on entry is correct. But setting
> only one of the two in the ordinary way is insufficient.

In what way is it insufficient? The nullity of dct_unquantize is not used 
anywhere that I can see. And if it were used, it would behave different 
depending on the availability of MMX which would most certainly not work.

Of course it would be *better* to also clean-up the denoise_dct pointer. If 
you have a better patchset coming, I can drop this one but otherwise I don't 
get your point here.

Rémi Denis-Courmont:
> Le sunnuntaina 9. kesäkuuta 2024, 11.39.55 EEST Andreas Rheinhardt a écrit :
>> Andreas Rheinhardt:
>>> Rémi Denis-Courmont:
>>>> On entry the function pointer is always NULL. We just need to set the
>>>> pointer before probing x86 CPU optimisations.
>>>
>>> Incorrect:
>>> https://github.com/mkver/FFmpeg/commit/d22d4ee8419788f9bb239a21e276cebce08
>>> 91737 (see also
>>> https://github.com/mkver/FFmpeg/commits/mpegvideo_pool/?after=d2dfcf8f226c
>>> 3708f3df080aed043ff4aa26e7cd+34 which contains the equivalent of patches
>>> 1+2 and a better version of #4)
>> Wait, I see that you only set dct_quantize unconditionally. So your
>> claim that dct_quantize is always NULL on entry is correct. But setting
>> only one of the two in the ordinary way is insufficient.
> 
> In what way is it insufficient? The nullity of dct_unquantize is not used 
> anywhere that I can see. And if it were used, it would behave different 
> depending on the availability of MMX which would most certainly not work.
> 

The two refers to the functions that are set by ff_dct_encode_init():
dct_quantize and denoise_dct (the other one). It does not involve
dct_unquantize at all.

> Of course it would be *better* to also clean-up the denoise_dct pointer. If 
> you have a better patchset coming, I can drop this one but otherwise I don't 
> get your point here.
> 

Ok. Will send my patchset (it is currently in a branch on top of my
other mpegvideo patchset, but actually it is not logically dependent on
the rest).

- Andreas

Le sunnuntaina 9. kesäkuuta 2024, 12.26.50 EEST Andreas Rheinhardt a écrit :
> The two refers to the functions that are set by ff_dct_encode_init():
> dct_quantize and denoise_dct (the other one). It does not involve
> dct_unquantize at all.

I don't think that the patchset implies any involvement of dct_unquantize 
though. There just so happens to be a contemporary patchset involving H.263 
DCT unquantize. So I still don't get your point.

Rémi Denis-Courmont:
> Le sunnuntaina 9. kesäkuuta 2024, 12.26.50 EEST Andreas Rheinhardt a écrit :
>> The two refers to the functions that are set by ff_dct_encode_init():
>> dct_quantize and denoise_dct (the other one). It does not involve
>> dct_unquantize at all.
> 
> I don't think that the patchset implies any involvement of dct_unquantize 
> though. There just so happens to be a contemporary patchset involving H.263 
> DCT unquantize. So I still don't get your point.
> 

You wrote about the "nullity of dct_unquantize" and it not being used at
all. So I inferred that you believe that my "the two" (as in "one of the
two") are dct_quantize and dct_unquantize. Which is not what I meant. I
meant dct_quantize and denoise_dct. So I don't get your latest answer at
all, given that I already stated that I did not refer to dct_unquantize
at all.

- Andreas