[FFmpeg-devel] [PATCH] Efficiently support several output pixel formats in Cinepak decoder

[u-9iep at aetey.se]

On Thu, Feb 02, 2017 at 04:25:05PM +0100, wm4 wrote:
> I can see how conversion in the decoder could speed it up somewhat
> (especially if limited by memory bandwidth, I guess), but it's not
> really how we do things in FFmpeg. Normally, conversion is done by
> libswscale (or whatever the API user uses).

I would not call "twice" for "somewhat" :)

The point is to do something that libswscale hardly can help with.

> > The format can be also chosen by setting environment variable
> > CINEPAK_OUTPUT_FORMAT_OVERRIDE, if this is enabled at build time.
> 
> It's not an environment variable, but a preprocessor variable. It's

Both are (intentionally) spelled the same which misled you to judge so.

> also not defined by default, which effectively makes some of your
> additions dead code. This is also not a thing we do in FFmpeg, and it

I am always enabling this feature anyway.
Was unsure whether activate it by default would be acceptable.

> usually creates maintenance nightmares. (For you too. Who would care
> about accidentally breaking this code when making changes to live parts
> of the decoder?)

Shall I repost the patch with this part on by default?

> > -typedef struct CinepakContext {
> > +typedef struct CinepakContext CinepakContext;
> > +struct CinepakContext {
> > +    const AVClass *class;
> >  
> >      AVCodecContext *avctx;
> >      AVFrame *frame;
> > @@ -71,24 +87,111 @@ typedef struct CinepakContext {
> >      int sega_film_skip_bytes;
> >  
> >      uint32_t pal[256];
> > -} CinepakContext;
> 
> Stray change?

No.

The compiler complained about referencing the type
being typedef'ed when I introduced function pointers into this
structure, the functions take pointers to it as args:

> > +    void (*decode_codebook)(CinepakContext *s,
> > +                            cvid_codebook *codebook, int chunk_id,
> > +                            int size, const uint8_t *data);
> > +    int  (*decode_vectors)(CinepakContext *s, cvid_strip *strip,
> > +                           int chunk_id, int size, const uint8_t *data);

> > +static void cinepak_decode_codebook_rgb32 (CinepakContext *s,
> > +    cvid_codebook *codebook, int chunk_id, int size, const uint8_t *data)
> 
> > +static void cinepak_decode_codebook_rgb24 (CinepakContext *s,
> > +    cvid_codebook *codebook, int chunk_id, int size, const uint8_t *data)
> 
> > +static void cinepak_decode_codebook_rgb565 (CinepakContext *s,
> > +    cvid_codebook *codebook, int chunk_id, int size, const uint8_t *data)

> So a part of the decoder is essentially duplicated for each format?

It is the irregular differences between them which are the reason
for splitting. I would not call this "duplication". If you feel
it is straightforward and important to make this more compact,
with the same performance, just go ahead.

> What we _especially_ don't do in FFmpeg is hardcoding colorspace
> conversion coefficients in decoders, and doing colorspace conversion in
> decoders.

Have you got a suggestion how to do avoid this in this case,
without sacrificing the speed?

> > +    char *out_fmt_override = getenv("CINEPAK_OUTPUT_FORMAT_OVERRIDE");

> Absolutely not acceptable.

1. Why?

2. I am open to a better solution of how to choose a format at run time
when the application lacks the knowledge for choosing the most suitable
format or does not even try to.

Any suggestion which can replace this approach?

> AFAIK we don't usually do INFO messages in decoders or anywhere outside
> of CLI tools. I'm probably wrong, but it should be avoided anyway.

This is very useful when you wish to check that you got it right
for a particular visual buffer / device, given that an apllication can
try to make its own (and possibly bad) choices. Not critical, I admit.

Regards,
Rune