[FFmpeg-devel] [PATCH] Cinepak: speed up decoding several-fold, depending on the scenario, by supporting multiple output pixel formats.

u-9iep at aetey.se u-9iep at aetey.se
Mon Feb 6 11:05:10 EET 2017


On Mon, Feb 06, 2017 at 07:57:25AM +0100, Clément Bœsch wrote:
> On Sun, Feb 05, 2017 at 12:24:30PM +0100, u-9iep at aetey.se wrote:
> > Hello,
> > 
> > Here comes an amended patch, I think all the relevant points
> > in the discussion have been addressed:
> > 
> > - maintainability and code duplication:
> >   straightforward code templating to reduce duplication
> >   would hardly improve its quality, robustness and maintainability;
> >   a proper style improvement is aking to a rewrite of the concerned
> >   functions instead of the reuse of the previous well tested code;
> >   if to be done, this should be done separately
> > 
> >   * left as-is (further rewrite, outside the scope of the patch)
> > 
> 
> No, code quality is not outside the scope of your patch.

Code quality is a subjective matter.

I believe it is as good as it has to, considering

 - correctness  (do you see any errors?)
 - efficiency   (where it sucks?)
 - readability  (is there any regression? I believe the patch makes
                 the decoder quite a bit more understandable)
 - robustness to partial modifications, i.e. constrained to one decoder
                (if I tweak one function, I do not have to bother
                 about the others, the interface is constrained
                 to the calling API, no massive side effects which
                 templating relies upon; what's wrong?)

What else?

Would you formulate it in a constructive/measurable way,
what is supposed to happen, to please you just enough?

> [...]
> > - use of environment variables to influence the behaviour of the
> >   libraries in ffmpeg is strongly discouraged
> > 
> >   * left disabled, as a reference/comment, being in certain situations
> >     (like those which motivated the optimizations) the only feasible
> >     solution
> > 
> 
> The use of the environment variable is not tolerable, this is a blocker.

It is explicitly specified that it is _not_ being used,
are you reading something else?

> [...]
> > Also added some comments with rationales.
> > 
> > I thank everyone for the feedback and hope this code can find its way
> > into upstream.
> > 
> 
> I'm sorry but there is no way it will reach upstream in this form.

As a matter of fact, before the posting I went out of my way and gave
it another try to refactor out the about twelve lines of code which are
duplicated between the ten functions (amounting to potentially reduce
the LOC count by about 10%). Unfortunately, the code was not written
from the beginning to suite for splitting.

This attempt did not lead to any pretty or better understandable/manageable
code, while introducing dependencies between the different decoding functions.

You or someone else here may be of course better qualified for such kind
of improvement, then feel free to propose the restructuring change, or why
not another way to do decoding three times faster on the same hardware.

Otherwise, if you just want to "avoid the duplication for any price" and
do not see a value in readability and maintainability (yes I want both,
a next change if any quite probably will be mine as well) then there is
of course no way to go past your block.

Regards,
Rune



More information about the ffmpeg-devel mailing list