Diego Biurrun <[email protected]> writes:
> On Wed, Mar 28, 2012 at 12:15:00PM +0100, Måns Rullgård wrote:
>> Diego Biurrun <[email protected]> writes:
>>
>> > Previously the JPEG DCTs were enabled for all encoders, now they are
>> > only selected for the handful of encoders that actually need them.
>>
>> Are you sure these are the only ones? Since the DCT is mostly accessed
>> through a function pointer, it would build even with the DCTs
>> erroneously disabled only to fail or crash at runtime.
>
> I ran this commit through my script that tests individual components.
> I added the dependency for all the encoders that failed to compile
> w/o it. What else do you want me to do?
Compile-testing does not prove anything. To see what I mean, imagine
deleting the code that sets the DSPContext.fdct pointer. This will
compile fine but will crash as soon as something tries to call it.
Codecs can and do use the dct without directly depending on any symbols
from the C files containing the dct implementations.
>> Moreover, since there are multiple choices of DCT, none of them are
>> strictly required.
>
> This is not currently correct, check the relevant bits in dsputil.c. The
> default and fallback is the JPEG DCT, which is not user-selectable.
There is also the faan dct, which is entirely independent from the jpeg
ones. All but a few encoders will work perfectly fine without the jpeg
dct if somehow the faan dct is activated in its place. This is a
runtime option even available on the avconv command line.
> It is currently enabled as soon as any encoder is enabled, thus
> strictly required for all encoders. After my patch it is required
> when an encoder that needs it is enabled.
Patently false.
>> > @@ -1373,8 +1375,8 @@ rv40_decoder_select="golomb h264chroma h264pred
>> > h264qpel"
>> > snow_decoder_select="dwt"
>> > -snow_encoder_select="aandct dwt"
>> > -svq1_encoder_select="aandct"
>> > +snow_encoder_select="aandct dwt jpegdct"
>> > +svq1_encoder_select="aandct jpegdct"
>>
>> Since when does snow use DCT?
>
> It is a transitive dependency from mpegvideo_enc.o. One could rightly
> argue that it's a sign of insufficient refactoring in those general
> encoding bits, but that is far outside the scope of this patch.
In that case your patch is making things worse. Who is going to find
and fix all the false dependencies you are adding? This is no easy task.
>> > --- a/libavcodec/dsputil.c
>> > +++ b/libavcodec/dsputil.c
>> > @@ -2780,7 +2780,7 @@ av_cold void ff_dsputil_init(DSPContext* c,
>> > AVCodecContext *avctx)
>> >
>> > -#if CONFIG_ENCODERS
>> > +#if CONFIG_JPEGDCT
>> > if (avctx->bits_per_raw_sample == 10) {
>> > c->fdct = ff_jpeg_fdct_islow_10;
>> > c->fdct248 = ff_fdct248_islow_10;
>> > @@ -2800,7 +2800,7 @@ av_cold void ff_dsputil_init(DSPContext* c,
>> > AVCodecContext *avctx)
>> > c->fdct248 = ff_fdct248_islow_8;
>> > }
>> > }
>> > -#endif //CONFIG_ENCODERS
>> > +#endif /* CONFIG_JPEGDCT */
>>
>> This is wrong. That ifdef covers all the DCTs, not only the jpeg ones.
>
> There is the FAAN DCT as well, which I need to have depend on the JPEG
> DCT, i.e. add
>
> faandct_select="jpegdct"
>
> to configure. I'll send an amended version soon.
That is so fucked up it is not even funny. PLEASE do not send any more
such patches until you have understood how this code works _and_ how it
can be truly made better.
--
Måns Rullgård
[email protected]
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
