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?
> 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. 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.
> > @@ -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.
> > --- 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.
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
