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

[wm4]

On Thu, 2 Feb 2017 16:59:51 +0100
u-9iep at aetey.se wrote:

> 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" :)

Well, your original mail mentioned only speedups up to 20%.

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

Why not?

> > > 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.

The heavy code duplication has other downsides. What if someone fixes
a bug, but only in the rgb32 version and ignores the rgb565 version?

> > 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?

Not for me.

> > 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?

Is there any value in this at all? It's a very old codec, that was
apparently used by some video games. What modern application of it
would there possibly be? And that in addition would require special
optimizations done by no other codec?

> > > +    char *out_fmt_override = getenv("CINEPAK_OUTPUT_FORMAT_OVERRIDE");  
> 
> > Absolutely not acceptable.  
> 
> 1. Why?

See the reply by BBB.

> 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?

get_format would be more appropriate.

> > 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.

Add that to your application code. Or alternatively, make ffmpeg.c
print the format (this would be useful for a few things).