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

[wm4]

On Mon, 6 Mar 2017 10:15:44 +0100
u-9iep at aetey.se wrote:

> On Sun, Mar 05, 2017 at 06:20:34PM -0500, Ronald S. Bultje wrote:
> > Hi Rune,
> > 
> > On Sun, Mar 5, 2017 at 4:26 PM, <u-9iep at aetey.se> wrote:
> >   
> > > Ronald,
> > >
> > > On Sun, Mar 05, 2017 at 02:38:31PM -0500, Ronald S. Bultje wrote:  
> > > > Hi,
> > > >
> > > > On Sun, Mar 5, 2017 at 2:22 PM, <u-9iep at aetey.se> wrote:
> > > >  
> > > > > On Mon, Feb 13, 2017 at 02:41:40PM +0100, u-9iep at aetey.se wrote:  
> > > > > > On Mon, Feb 13, 2017 at 02:19:45PM +0100, Michael Niedermayer wrote:  
> > > > > > > you may want to add yourself to MAINTAINERs (after talking with
> > > > > > > roberto, who i belive has less interrest in cinepak than you do
> > > > > > > nowadays)  
> > > > > >
> > > > > > Sounds ok for me. Roberto, what do you think (if you read this)?  
> > > > >
> > > > > The only address to him which I found (in an old commit) bounced,
> > > > > there was no reply here on the list either.
> > > > >
> > > > > Both can be a coincidence, but otherwise it looks like the change  
> > > should  
> > > > > be OK.  
> > > >
> > > >
> > > > No. This has been discussed repeatedly. Stop trying to push this through.  
> > >
> > > My maintainership (for the code which I have contributed to, you may be
> > > unaware about this fact) was not discussed otherwise than cited here.
> > >
> > > Please check what you are commenting,
> > > especially when you mean to sound like having a definite power  
> > 
> > 
> > The rule is that a patch cannot be committed while a developer has
> > outstanding comments. There's outstanding comments, including some from me.
> > You said "the change should be OK", and I'm simply saying "no" to that,
> > because it's not. The patch is not OK until review comments from other
> > developers have been addressed by the patch submitter.  
> 
> Thanks for the explanation Ronald,
> 
> 1. Apparently you did not aim at the maintainership question.
> 
>   It would be nice if you confirm this point,
>   to avoid further misunderstandings.
> 
> 2. Would you cite the outstanding comment or comments about the patches
>    which you feel are valid and not addressed?
> 
>    I kindly ask you check the latest iteration of the patch series
>    where I tried to summarize all discussion points in the containing
>    letter.
> 
> TL;DR: I do have respect for your work and competence.
>        Please do the same to others. Even if we all too often meet people
>        who lack in those areas, there are some exceptions.
> 
> To be fair your comments concerning the patches were among the most
> detailed and friendly, this is appreciated!
> 
> But even your well meant comments happened to miss the point, being based
> in unfounded assumptions. I answered and explained and there it stopped.
> 
> Frankly the only outstanding comments which I am aware of are of the kind
> "you the patch submitter do not understand why 'this' is better than
> 'that'".
> 
> The fact is that I _do_ understand why you believe that something is
> better or not. I just do not feel a belief is sufficient per se.
> 
> I invited you and others to look at _what_ makes something better
> or not and got literally no answers.
> 
> Besides of course
> "imagine if someone else will do something else,
> it would be terrible, thus you are leading us to hell" :)
> 
> or otherwise, citing literally:
> "we know this code, we know it can do this, don't tell us it's not
> possible" without specifying (and in fact misunderstanding) what
> "this" we were talking about: making the speedup usable with an
> unprepared application, which the overhelming majority of applications
> are. Regrettably, the present code is not prepared nor meant to be able to.
> The proposed code could, but this possiblity is now cut off,
> just to avoid contention.
> 
> As a third example, your comment
> "We [...] know it's unreasonable from a maintenance perspective".
> 
> The very presence of the cinepak decoder is a proof to the opposite
> because it worked this way (generating two different pixel formats
> inside the decoder) for years. Again, what "it's" we are talking about?
> The same "it" in your mind as in the patch?
> 
> I went so long as to quantify/estimate the expected extra maintenance
> burden while you did not even mention any tangible criteria.
> 
> This makes me doubt that you or others who commented have time to read
> the answers or are prepared to expect competence of your counterparts.
> Unfortunately this affects the quality of the judgement.
> 
> I do have respect for your work and competence.
> Please do the same to others.

I think we've repeatedly made clear that:

- we don't really want the decoder to output multiple pixfmts by choice
- but if it's limited to a very small number of formats (say, 2) it
  might be ok
- but ideally the decoder should output the "native" pixfmt/colorspace
  of the codec, which apparently is YCgCo (which would also require
  libswscale modifications)
- we're not even interested in a faster cinepak decoder, because we see
  no need for it (even you haven't demonstrated any need for it - I'm
  pretty sure everyone would be all over a fast intermediate codec, but
  people don't use cinepak for that)
- trying to trick us into applying this patch by playing policy lawyer
  won't work
- whether or not having these multiple code paths would be so horrible
  is not even the main question - by now we're just annoyed by this
  topic and how much attention it seems to drain, so we'd rather ignore
  this (don't blame us - people tend to ignore unimportant things to
  get important work done, and not applying your patches seems to be
  the safer option)
- again, nobody understands your needs, and some we find extremely
  absurd and out of place, like using getenv() in a decoder (we'd
  probably suggest a better alternative if we did, maybe)