[FFmpeg-devel] End of involvement (was: [PATCH] Cinepak: speed up decoding several-fold...)

[u-9iep]

Now it has been more than a month since the initial submission
of the Cinepak decoder speedup patch (not counting the proof of concept
posted two months ago).

Since then, more and more of the oftentimes ignorant discussion was
dedicated to the perceived design/development policy conformance (still
did not see any reference to a corresponding document :) with no outcome.

Given the apparent lack of the interest from the FFmpeg project in making
improvements to the lightweight fast codecs (the class Cinepak belongs to)
and my lack of appreciation for the conversational style on the list
I do not feel motivated to contribute to the project any longer.

My thanks for FFmpeg and for the enormous work on making it as useful
as it is go to Michael Niedermayer, who should be also credited for his
much appreciated support and work to allow the integration into FFmpeg
and the past improvements of Cinepak encoder and decoder.
(To be clear: the friendly support does not imply endorsement)

Thanks Michael.

Now I end my involvement with FFmpeg, good luck with the project.

If a casual reader interested in Cinepak would try the patches,
I recommend the series
  https://ffmpeg.org/pipermail/ffmpeg-devel/2017-February/207603.html
  https://ffmpeg.org/pipermail/ffmpeg-devel/2017-February/207597.html
including
  https://ffmpeg.org/pipermail/ffmpeg-devel/2017-February/207625.html

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] (was Re: deduplicated [PATCH] Cinepak: speed up decoding several-fold...) formats.

[u-9iep]

Hello Karl,

I appreciate your interest and comments.

Keeping on topic, to the patch decision makers:

 I actually complied with all real suggestions heard during
 the discussion, even with those I find being misdirected,
 i.e. virtually everything except dropping the patch as a whole.

 Now I am unsure, will anybody look at the patch
 or is it so that some policy (which everyone here but me knows)
 does clearly specify that such a patch shall be rejected no matter what?

Possibly the dialogue below will be ok ontopic, as a background.

On Mon, Mar 06, 2017 at 05:42:31PM +0100, Karl Blomster wrote:
> >Nor is there any apparent reasonable way to "get rid" of those getenv()s.
> I would say that as usual when determining what is "right" in software
> architecture, the answer is "it depends". As far as I am aware, most of the

Thank you. I fully agree.

> things libav* touches environment variables for is related to interactions
> with things external to ffmpeg, such as the network (HTTP_PROXY), the
> terminal, the locations of certain types of plugins, etc. Modifying internal
> behavior based on environment variables is ugly in my opinion, and I can't
> think of any other case in ffmpeg where this is done. In general, if you
> want to interact with library functionality, do it through the API. In this
> case there's even a mechanism in place explicitly for this purpose
> (negotiating output pixelformat via the get_format callback), although I'll
> note that the ordinary use case for that is quite different from what you
> want to do here.

Exactly, the possibility to choose the pixel format is present but it
is not meant to be controlled otherwise than via a competent application.

I agree that an added envvar is by nature an intrusive change,
that's why I try to reduce the tention by omitting this
(I have to patch my own ffmpeg builds in any case).

> As far as SDL goes, I'll just note that the documentation states that
> "[u]sing these variables isn't recommended and the names and presence of
> these variables aren't guaranteed from one release to the next." They seem
> intended for debugging only.

I guess they _were_ thought for debugging while one did not realize
that some things are impossible or hard to put under the applications'
responsibility. They have since then in practice become an integral part
of the product. My opinion corresponds to the fact that the variables
always are present in the usual non-debug SDL builds, do you have a
different experience? Of course I can not talk for the usage od SDL
everywhere, my use of the SDL-bound applications would be hardly
meaningful without the variables.

> >The cost in the code is in fact not that "big", about 68 LOC per output
> >pixel format, in the latest revision. Compared to the several times (!)
> >speed loss when going through swscale it looks to me like an
> >exceptionally low hanging fruit.
> And here's one of the biggest issues in this debate and where I think you
> and wm4 et al keep talking past each other. The cost in lines of code is
> relatively unimportant. The cost in "number of different behaviors" is
> different.

Ah. I see your point. Each extra degree of freedom makes it harder
to support all the possible cases and combinations.

> In practice this isn't a huge deal for cinepak in particular
> since it already does this bit of ugliness, but surely you can see why there
> is a reluctance to add more of it?

Thanks. Pity no one else did say this. Fortunately the last version
of the patch definitely does not make the situation worse, rather
quite more regular - pal8 was handled as a special case (a very rare one,
which makes maintenance even harder, also one which made the output depend
on the format of input), rgb565 is fast, fully regular and always available
(and also well defined what it shall look like, no uncertainty like yuv).

0rgb or argb are full quality and faster than rgb24 on usual hardware
but replacing the default rgb24 format would introduce a surprise.

> Then there's also the fact that, to be blunt to the point of rudeness,
> nobody *cares* about cinepak. I'm sure that *you* do, and as you've
> repeatedly pointed out we do not have your point of view, but from the
> "mainstream" perspective or whatever you want to call it this is definitely
> a fringe codec that has very few users today. Hence the reluctance to accept

I agree, it is not so well visible as the newest heavy codecs. It is not
profitable to the CPU/GPU manufacturers nor suits the contents providers,
they _choose_ to copy every byte again and again as many times as it is
being played. That's why the compression ratio becomes so important,
because the user not the provider pays for the heavy decoding while the
provider can not avoid paying for the (accumulated) traffic, nor accepts
to optimize the transmission by decoupling the user from the service
(say, like bittorrent).

The applications using cinepak do still exist but it is seldom advertised.

OTOH 

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

[u-9iep]

On Mon, Mar 06, 2017 at 04:31:32PM +0100, wm4 wrote:
> On Mon, 6 Mar 2017 16:23:15 +0100
> u-9...@aetey.se wrote:
> 
> > Did the project (who?) ever make a general decision about Cinepak
> > or delegate to wm4 to represent the project's stance?
> > 
> > I question her/his tone, not the policy, but what is the latter
> > when not refracted through your, wm4's or someone else's interpretation?
> > 
> > The design decision you refer to I would love to read. What I have
> > seen on the ffmpeg.org did not say "decoders shall not produce multiple
> > pixel formats".
> 
> Before you get too obsessed with me, I have never represented anything,
> or intended to represent. I merely contributed my own opinion about
> these patches. I have not more powers than Paul Mahol or Ronald Bultje
> (if anything, I have less powers than them).

It is nice of you to say this. Thanks for the clarification.

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

On Mon, Mar 06, 2017 at 08:19:06AM -0500, Ronald S. Bultje wrote:
> On Mon, Mar 6, 2017 at 7:08 AM,  wrote:
> 
> > It is clear that you personally do not want the Cinepak decoder to be able
> > to output multiple pixel formats, for unspecified reasons ("by choice").
> >
> > You make it look like it is your discretion who decides whether something
> > is deemed to be good or not for FFmpeg. I do not believe this.

> It is a general project design decision.

Did the project (who?) ever make a general decision about Cinepak
or delegate to wm4 to represent the project's stance?

I question her/his tone, not the policy, but what is the latter
when not refracted through your, wm4's or someone else's interpretation?

The design decision you refer to I would love to read. What I have
seen on the ffmpeg.org did not say "decoders shall not produce multiple
pixel formats".

> If you want to change that, fine, propose it and let's discuss that on its

I am really interested to know what is "that" you refer to.

> own merit. But right now this is how the project works. You can't change
> that without changing official project policy.
> 
> I'm fundamentally against it, and as such I'm against the patch. I
> understand why you want it, but I don't think that's sufficient reason.

Thanks for stating your position as you do, constructively and coherently.

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

On Mon, Mar 06, 2017 at 08:19:06AM -0500, Ronald S. Bultje wrote:
> Hi,
> 
> On Mon, Mar 6, 2017 at 7:08 AM,  wrote:
> 
> > You did not have to pay attention to the patch, given your limited
> > understanding of the matter
> 
> 
> And this is a CoC [1] violation, please don't do that again.

Actually what I wrote is:

> > You did not have to pay attention to the patch, given your limited
> > understanding of the matter (which _you_ stated repeatedly) 

I am sorry for saying this.

Pointing to one's behaviour is correct, presenting it as one's inherent
quality is not. I did wrong.

Indeed that person did not state her/his limited understanding nor
did this repeatedly. What was repeated was erroneous technical
comments about which "native" pixel format Cinepak has.

The formulation about understanding I meant to refer to was that of wm4:

  > I don't know what you're thinking, but that's just wrong.

It looks bizarre to me to judge without knowing
but this was _not_ a confession of a "limited understanding of the matter".

I am sorry for misrepresentation of the statements of wm4!

Now Ronald where were you with your policing
when the person in question (wm4) repeatedly violated CoC?

I point out that your behaviour of selective punishment
is most probably not conscious but it is a fact.

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] (was Re: deduplicated [PATCH] Cinepak: speed up decoding several-fold...) formats.

[u-9iep]

Karl,

I wish to thank you for the effort to make the "opposite" parties
to better understand each other.

As you say, indeed the patch makes a move which looks in line with
ffmpeg's former traditions but which at least formally collides with
the currently percepted "best practices".

On Mon, Mar 06, 2017 at 12:47:26PM +0100, Karl Blomster wrote:
> think the original patch made this seem particularly unpalatable since it
> added a whole new "quick and dirty" colorspace conversion to YUV with
> hardcoded constants (there are several different YUV variants with slightly
> different constants), with a intended-as-helpful link to the Wikipedia page

I am pretty sure the definition of yuv420p in ffmpeg and the constants
agree to each other. Yes I am aware of the mess around the variety of
YUVs, that's why I referred to the source of the numbers. (IIRC they
come actually from Microsoft but I felt Wikipedia looked more neutral)

> I really don't think that link was appreciated at all.

Interesting that nobody but you could articulate what was wrong with
it. Anyway, this detail is now a matter of the past. I never needed
it in practice myself (hardware scalers via yuv420p were slower than
Cinepak-on-the-CPU), implemented it just for generality.

Undesired - removed.

> With all of this in mind, there would need to be an extraordinarily big
> practical benefit for a large number of users for this patch to be percieved
> as a "necessary evil" (and just to be perfectly clear here, I'm not trying

My problem with the commenters is that they take their percepted best
practice rules as something which is not to be exposed to scrutiny.

An attempt to say that the rules are not necessary applicable or useful
is not heard at all or otherwise amounts to heresy.

> correspondence, my impression gathered from several different hints in
> different emails is that you're trying to play back video on some
> extraordinarily slow system with no GPU, where the only decoder you could

Correct, no GPU. (GPU costs among others a lot more lines of code to
take care of than Cinepak patches :)

> cannot modify the playback software, necessitating the rather remarkable
> hack where a ubiquitous system library modifies its behavior based on an

Do you believe that libraries are not supposed to react on environment
variables? The most ubiquitous system one (libc) does by design.

Ffmpeg itself contains over twenty getenv() in the libav* libraries and
also via ffplay depends on SDL which for very natural reasons (similar
to my motivation with Cinepak) relies on environment variables.
Nor is there any apparent reasonable way to "get rid" of those getenv()s.

> ffmpeg
> is a generalist library that tries to support a broad range of use cases on
> a wide range of platforms, but that also by necessity means it lends itself
> less well to extremely specific use cases like this one, where you're
> prepared to buy decoding performance at any cost.

The cost in the code is in fact not that "big", about 68 LOC per output
pixel format, in the latest revision. Compared to the several times (!)
speed loss when going through swscale it looks to me like an
exceptionally low hanging fruit.

> In an earlier email you pointed out that historically, very few code changes
> have been needed to keep the cinepak decoder up to date with the rest of
> ffmpeg, highlighting a view of what maintainability is that I believe is
> substantially different from that of other participants in this debate.

Nobody cared to define their view on what maintainability means
even when I explicitly asked how to measure it.

> However, what it does imply is that the maintenance burden of having your
> own branch that suits your use case, with all the oddball solutions you
> could imagine, would not seem to be overly heavy.

That's what I was doing and apparently will have to, given that the
vitally useful things like out-of-band control are percepted as "evil" here.

It still feels inadequate to keep this functionality for myself, or
otherwise to maintain the patches so that they would suit someone else
(I made this now for the submission but would not do continuously).

> Also, in another of your earlier emails you also rather passive-aggressively
> mentioned that not accepting the patch might be a "problem for the project".
> I really don't think anyone around here sees it that way, precisely because
> interest in cinepak in 2017 is not, shall we say, at an all time high.

Indeed it is not the speed of Cinepak, nor the speed of any module
which is a problem, but I would say the atmosphere in the project.

> Best regards
> Karl Blomster

Thanks again.

Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

To everyone:

I am really sorry for having to react to this kind of irrational
arguments. OTOH keeping silence could be interpreted as accepting them.

As far as my common sense goes, I can not count these as "pending comments".

TL;DR:
 trying to reason,
 given the arbitrary statements and careless behaviour of the opponent

On Mon, Mar 06, 2017 at 10:31:42AM +0100, wm4 wrote:
> I think we've repeatedly made clear that:
> - we don't really want the decoder to output multiple pixfmts by choice

It is clear that you personally do not want the Cinepak decoder to be able
to output multiple pixel formats, for unspecified reasons ("by choice").

You make it look like it is your discretion who decides whether something
is deemed to be good or not for FFmpeg. I do not believe this.

> - but if it's limited to a very small number of formats (say, 2) it
>   might be ok

You are again referring to nothing but your discretion.

Anyway, if you take your time to look at the current version of the patch,
it _happens_ to do exactly this.

So this point is moot.

> - but ideally the decoder should output the "native" pixfmt/colorspace
>   of the codec, which apparently is YCgCo (which would also require
>   libswscale modifications)

What makes it "ideal", by which criteria? Be technical please.

(BTW "which apparently is" is a wrong assumption.
You still did not use the references I offered you for some background
information on the matter. Please learn, before posting next time.)

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

Are you saying no need for everyone?

Nor have they ever tasted a 3 times faster decoder but you already know
they will not like it?

Sorry, seriously I have no reason to assume that you have prerequisites
to know for everyone and especially in advance.

You are talking about what is inside the horizon of yourself.

> - trying to trick us into applying this patch by playing policy lawyer
>   won't work

You make it again sound like you decide for the project.
Frankly, such behaviour makes the project look bad.

Also you are accusing me of "tricking" and "playing".

Please be specific about what I am doing wrong
or apologize otherwise.

While at it, I am still expecting your apology
for the use of abusive language when you were commenting
on my proposal last time.

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

You did not have to pay attention to the patch, given your limited
understanding of the matter (which _you_ stated repeatedly) and the
limited capacity to participate, which you demonstrated by relying on
wrong assumptions and neglecting to correct your mistakes in this discussion.

> - again, nobody understands your needs, and some we find extremely

You are assuming that everyone has the same level of understanding
as you do. This is not necessarily true.

>   absurd and out of place, like using getenv() in a decoder (we'd
>   probably suggest a better alternative if we did, maybe)

Regrettably, the proposed alternatives were either to put the
corresponding functionality into every application or to avoid ffmpeg
and start a separate project. Neither of those alternatives would be
practically useful. Never mind, this is not part of the current patch.

Thanks for reading this long!

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

On Mon, Mar 06, 2017 at 10:19:33AM +0100, Paul B Mahol wrote:
> On 3/6/17, u-9...@aetey.se  wrote:
> > I do have respect for your work and competence.
> > Please do the same to others.
> 
> What is your Work and Competence in FFmpeg?

This is offtopic and looks like ad hominem
"a logical fallacy in which an argument is rebutted by attacking the
character, motive, or other attribute of the person making the argument"
(Wikipedia)

It is more of a fallacy because you make it look like the "competence
in FFmpeg" (whatever you mean with this) is the only competence kind to
be respected or/and to be relevant while doing work for FFmpeg.

As for your curiosity, feel free to do the research.
The data is public.

Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

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,  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,  wrote:
> > >
> > > > On Mon, Feb 13, 2017 at 02:41:40PM +0100, u-9...@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.

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

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,  wrote:
> 
> > On Mon, Feb 13, 2017 at 02:41:40PM +0100, u-9...@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 and being
quite rude.

What _has_ been discussed are the proposed Cinepak decoder improvements.

There has not been even a single substantial technical argument against
any version of the patch, nor any style/duplication argument against
the last two versions. (Did you read the discussion? Did you check the
validity of the presented arguments from all the involved parties, yours
truly included? You did not say a word after I addressed your concerns.)

This makes it even harder to put your statement into a proper context.

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

On Mon, Feb 13, 2017 at 02:41:40PM +0100, u-9...@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.

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] OT (Re: [PATCH 1/2] (was Re: deduplicated [PATCH] Cinepak: speed up decoding several-fold...) formats.)

[u-9iep]

On Sun, Mar 05, 2017 at 07:32:08PM +0100, Paul B Mahol wrote:
> On 3/5/17, u-9...@aetey.se  wrote:
> > I kindly ask the reader to note that cinepak is not ffmpeg's everyday meal
> > i.e. fast shortcut judgements may be not applicable. Please take your time.

> Cinepak is old crappy codec. Get over it.

Paul, please be technical.

For your information, what you say is an emotional expression
which looks patronizing and unfriendly.

To everyone: sorry for offtopic (hopefully a somewhat useful one)

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 2/2] (was Re: deduplicated [PATCH] Cinepak: speed up decoding several-fold...) formats.

[u-9iep]

Whitespace adjustments only.

Regards,
Rune
>From 664e8878aac9dd9fa0393a1d60de81df8bf2f195 Mon Sep 17 00:00:00 2001
From: Rl 
Date: Sun, 5 Mar 2017 16:32:58 +0100
Subject: [PATCH 2/2] libavcodec/cinepak.c: small whitespace cleanups

---
 libavcodec/cinepak.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c
index d8bc7860eb..8434eaf35c 100644
--- a/libavcodec/cinepak.c
+++ b/libavcodec/cinepak.c
@@ -146,12 +146,12 @@ static int cinepak_decode_vectors_##pixel_format 
(CinepakContext *s, cvid_strip
 for (x=strip->x1; x < strip->x2; x+=4) {\
 if (selective_update && !(mask >>= 1)) {\
 if ((data + 4) > eod) return AVERROR_INVALIDDATA;\
-flag  = AV_RB32 (data); data += 4; mask = 0x8000;\
+flag = AV_RB32 (data); data += 4; mask = 0x8000;\
 }\
 if (!selective_update || (flag & mask)) {\
 if (!v1_only && !(mask >>= 1)) {\
 if ((data + 4) > eod) return AVERROR_INVALIDDATA;\
-flag  = AV_RB32 (data); data += 4; mask = 0x8000;\
+flag = AV_RB32 (data); data += 4; mask = 0x8000;\
 }\
 if (v1_only || (~flag & mask)) {\
 POINTER_TYPE *p;\
@@ -310,7 +310,7 @@ static int cinepak_decode_strip (CinepakContext *s,
  cvid_strip *strip, const uint8_t *data, int 
size)
 {
 const uint8_t *eod = (data + size);
-int  chunk_id, chunk_size;
+int chunk_id, chunk_size;
 
 /* coordinate sanity checks */
 if (strip->x2 > s->width   ||
@@ -426,7 +426,7 @@ static int cinepak_decode (CinepakContext *s)
 return result;
 
 s->data += strip_size;
-y0= s->strips[i].y2;
+y0 = s->strips[i].y2;
 }
 return 0;
 }
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 1/2] (was Re: deduplicated [PATCH] Cinepak: speed up decoding several-fold...) formats.

[u-9iep]

For one week there was no substantial feedback on the patches.

I understand that the first patch was very large which makes it hard
to review.

That's why I now have produced a limited, minimalistic change which still
yields most of the improvement.

I kindly ask the reader to note that cinepak is not ffmpeg's everyday meal
i.e. fast shortcut judgements may be not applicable. Please take your time.

To avoid contention around some of the issues which triggered such
regrettable kind of judgements let me point out that

- letting a decoder produce multiple pixel formats is explicitly
  allowed and supported by the documented API (get_format())

- the patch does not introduce a new feature in this respect,
  just makes its presence more visible and much more useful

- the cinepak codec is extraordinary well suited to produce virtually
  any desired pixel format very efficiently (iow it is quite special)

- even though we have got a framework of libswscale, there is no point
  to use it where it can not help; it would be several times slower than
  cinepak itself, NOTE not by a coincidence but by the very design of both

- when some functionality is desired from libswscale, it remains fully
  usable with cinepak, with no loss in the actual efficiency of libswscale

- the 3-fold practical speedup makes the difference between working/useful
  and unusable/useless, with very little complexity (aka maintenance costs)

- the feedback concerning the coding style and code duplication was not
  specified in any measurable terms but it seems that all related issues
  have been resolved

Looking forward to your analysis and feedback.

Thanks.

Rune
>From cbe0664a13a615ecac302ffb0de577cd08b1f910 Mon Sep 17 00:00:00 2001
From: Rl 
Date: Sun, 5 Mar 2017 15:54:06 +0100
Subject: [PATCH 1/2] Cinepak decoding: refactor for several-fold speedup

Refactored codebook generation and vector parsing
to better support the decoder API and allow for optimizations.

Decoding to rgb24 is now slightly faster.

Replaced generation of the constrained pal8 output
(which was only possible with palettized input)
with rgb565, comparably fast but working with any input format.

Decoding to rgb565 is now several-fold faster:
 (including overheads, underestimation of the actual decoding speedup)
 
 matrixbench_mpeg2.mpg (720x567) encoded with ffmpeg into Cinepak
 using default settings, decoding on an i5 3570K, 3.4 GHz:
 ...
 fast_bilinear:  ~65x realtime
 patch w/rgb565 override:~154x realtime
 
 https://ffmpeg.org/pipermail/ffmpeg-devel/2017-February/206799.html

The default output format is unchanged, rgb24.
---
 libavcodec/cinepak.c | 462 ---
 1 file changed, 252 insertions(+), 210 deletions(-)

diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c
index d657e9c0c1..d8bc7860eb 100644
--- a/libavcodec/cinepak.c
+++ b/libavcodec/cinepak.c
@@ -29,8 +29,8 @@
  * @see For more information on the quirky data inside Sega FILM/CPK files, 
visit:
  *   http://wiki.multimedia.cx/index.php?title=Sega_FILM
  *
- * Cinepak colorspace support (c) 2013 Rl, Aetey Global Technologies AB
- * @author Cinepak colorspace, Rl, Aetey Global Technologies AB
+ * Cinepak colorspace/optimizations (c) 2013-2017 Rl, Aetey Global 
Technologies AB
+ * @author Cinepak colorspace/optimizations, Rl, Aetey Global Technologies AB
  */
 
 #include 
@@ -39,24 +39,29 @@
 
 #include "libavutil/common.h"
 #include "libavutil/intreadwrite.h"
+#include "libavutil/opt.h"
+#include "libavutil/pixdesc.h"
 #include "avcodec.h"
 #include "internal.h"
 
+#define MAX_STRIPS  32/* an arbitrary limit -- rl */
 
-typedef uint8_t cvid_codebook[12];
-
-#define MAX_STRIPS  32
+typedef union cvid_codebook {
+uint8_trgb24[256][12];
+uint16_t  rgb565[256][ 4];
+} cvid_codebook;
 
 typedef struct cvid_strip {
 uint16_t  id;
 uint16_t  x1, y1;
 uint16_t  x2, y2;
-cvid_codebook v4_codebook[256];
-cvid_codebook v1_codebook[256];
+cvid_codebook v4_codebook;
+cvid_codebook v1_codebook;
 } cvid_strip;
 
-typedef struct CinepakContext {
-
+typedef struct CinepakContext CinepakContext;
+struct CinepakContext {
+const AVClass *class;
 AVCodecContext *avctx;
 AVFrame *frame;
 
@@ -71,195 +76,233 @@ typedef struct CinepakContext {
 int sega_film_skip_bytes;
 
 uint32_t pal[256];
-} CinepakContext;
-
-static void cinepak_decode_codebook (cvid_codebook *codebook,
- int chunk_id, int size, const uint8_t 
*data)
-{
-const uint8_t *eod = (data + size);
-uint32_t flag, mask;
-int  i, n;
-uint8_t *p;
-
-/* check if this chunk contains 4- or 6-element vectors */
-n= (chunk_id & 0x04) ? 4 : 6;
-flag = 0;
-mask = 0;
-
-p = codebook[0];
-for (i=0; i < 256; i++) {
-if ((chunk_id & 0x01) && !(mask >>= 1)) {
-

Re: [FFmpeg-devel] [PATCH] (for discussion): cuvid: allow to crop and resize in decoder

[u-9iep]

On Sat, Mar 04, 2017 at 08:33:03PM +0100, Timo Rothenpieler wrote:
> >Which criteria would make a decoder (or any tool) a wrong place
> >for something it does much better than anyone else?
> 
> It's about having scaling-functionality in libavcodec, while it belongs into
> libavfilter, but the cuvid API does not offer that possibility.

You take for granted "it belongs 'there'" but my question was not about
"where" but "why".

In these particular cases (cuvid, cinepak) a lib can perform at
best only a small fraction as good as the decoder itself.

So, again, what is our criteria to choose the most suitable place?

libxxx exist for a good reason, in many cases they are best as
providers of a certain functionality, compared to multiple spread ad-hoc
implementations.

OTOH when they are _not_ good at providing a functionality, and for
fundamental reasons can not be made as good as an alternative,
then why insist on using them?

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] (for discussion): cuvid: allow to crop and resize in decoder

[u-9iep]

On Sat, Mar 04, 2017 at 09:16:30AM -0800, Philip Langdale wrote:
> On Wed, 1 Mar 2017 11:58:39 +0100
> Timo Rothenpieler  wrote:
> > Would like to bring this back up.
> > I'd like to merge this, as specially the scaling is freely done by the
> > video asic, offering a possibility to scale without requiring non-free
> > libnpp. And cropping so far is not possible at all.
> > 
> > Yes, scaling and cropping is not something a decoder usually does, but
> > it exposes a hardware feature that has no other way of accessing it,
> > which offers valuable functionality to users.
> 
> I'm ok with it. I agree it's ugly, but if this is the only way, so be
> it.

I find it kind of intriguing that doing an operation at a place
where it is most efficient (also where it seems to belong by the codec
or hardware design) is being called "ugly".

> For what it's worth. there's precedence in crystalhd. I exposed the
> hardware's ability to do downscaling, which was valuable because it
> allowed you to downscale before memcpy, which made the difference
> between playable and unplayable for some low end machines.

The cinepak decoder is another precedent of the same kind, even if not
regarding scaling but pixel formats.

"Doing the operation where it costs least" looks like a reasonable
criteria, doesn't it?

Which criteria would make a decoder (or any tool) a wrong place
for something it does much better than anyone else?

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

This extra patch "beyond the series" is being posted for the benefit
of a casual reader who needs extremely fast/lightweight video decoding
of prearranged videos, with existing or/and mainstream applications
(e.g. like mplayer).

This patch is vital to make the cinepak decoding speedup (the matter
of the series) usable in practice.

(Otherwise you will have to modify the corresponding applications,
separately each of them, or if applicable build and use a preload hack
implementing an equivalent of this patch, as was suggested elsewhere on
this list.)

Such an out-of-band control was not welcome in ffmpeg even if disabled
by default, the project decision makers did not see the need.

That's why this patch is being posted separately, for those who do.

Enjoy!
Rune
>From 43aaba5a7fbf5f97fb7f50c4d1f48363d74332f8 Mon Sep 17 00:00:00 2001
From: Rl 
Date: Sat, 25 Feb 2017 15:35:44 +0100
Subject: [PATCH] Cinepak decoding: provide out-of-band control of output pixel
 format.

Allow the output pixel format to be chosen at runtime
by setting CINEPAK_DECODE_SET_PIXFMT envvar to the needed value.

This works with any application and complements the functionality
of get_format() API.

The existing applications are not prepared and generally are
not capable to choose the decoder output format correctly.
They often lack the information needed to be able to pick the format.
That's why get_format() is not sufficient.

Note that the proper choice of the most efficient output format is
not decoder- nor application-specific but usage-case-specific.

The only alternative to implementing this out-of-band format control
in the decoder library would be to modify every application
to include
 - either additional logic, reflecting a specific usage case
 - or an out-of-band control channel
neither of which is efficient or possibly not even feasible.
---
 libavcodec/cinepak.c | 35 +++
 1 file changed, 35 insertions(+)

diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c
index e32b07ce8a..d18b455589 100644
--- a/libavcodec/cinepak.c
+++ b/libavcodec/cinepak.c
@@ -916,6 +916,9 @@ static const enum AVPixelFormat pixfmt_list_2[] = {
 static av_cold int cinepak_decode_init(AVCodecContext *avctx)
 {
 CinepakContext *s = avctx->priv_data;
+#ifndef DISABLE_CINEPAK_DECODE_SET_PIXFMT
+char *out_fmt_override = getenv("CINEPAK_DECODE_SET_PIXFMT");
+#endif
 
 /* we take advantage of VQ to efficiently support
  * multiple output formats */
@@ -929,6 +932,38 @@ static av_cold int cinepak_decode_init(AVCodecContext 
*avctx)
 /* check for paletted data */
 s->palette_video = (avctx->bits_per_coded_sample == 8);
 
+#ifndef DISABLE_CINEPAK_DECODE_SET_PIXFMT
+/*
+ * Checking an environment variable for out-of-band control
+ * of the output pixel format:
+ *
+ * get_format() does _not_ help when you can not modify the applications
+ * to use it, let alone to use it appropriately under varying practical
+ * curcumstances, there is no general criteria capable to choose
+ * the most suitable pixel format for each case;
+ * that's why the availability of an out-of-band control channel
+ * is important, sometimes there is no alternative at all -- rl
+ */
+
+if (out_fmt_override && *out_fmt_override) {
+if (   !strcmp(out_fmt_override, "rgb32")) {
+avctx->pix_fmt = AV_PIX_FMT_RGB32;
+} else if (!strcmp(out_fmt_override, "rgb24")) {
+avctx->pix_fmt = AV_PIX_FMT_RGB24;
+} else if (!strcmp(out_fmt_override, "rgb565")) {
+avctx->pix_fmt = AV_PIX_FMT_RGB565;
+} else if (!strcmp(out_fmt_override, "yuv420p")) {
+avctx->pix_fmt = AV_PIX_FMT_YUV420P;
+} else if (!strcmp(out_fmt_override, "pal8")) {
+avctx->pix_fmt = AV_PIX_FMT_PAL8;
+} else {
+av_log(avctx, AV_LOG_ERROR, "Unsupported pixel format override 
'%s'\n",
+out_fmt_override);
+return AVERROR(EINVAL);
+}
+} else
+#endif
+
 if (s->out_pixfmt != AV_PIX_FMT_NONE) /* the option is set to something */
 avctx->pix_fmt = s->out_pixfmt;
 else
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

On Sat, Feb 25, 2017 at 09:27:05PM +0100, Paul B Mahol wrote:
> On 2/25/17, u-9...@aetey.se  wrote:
> > Hello,
> >
> > Here comes the latest version of the patch, with adjustments
> > made according to all substantial feedback comments.
> >
> > Among others the code has been further deduplicated to ease maintenance.
> >
> > Hopefully the 2-4 times improvement of the decoding speed justifies the
> > growth of the affected source file (from 491 to 979 lines) and also the
> 
> Unacceptable.

Any comments on the criteria?

Is no code growth acceptable no matter what, or is there possibly
some percentage which would be ok from your perspective for such a speedup?

There are ways to look for a common ground:

It would be trivial to cut the code, cutting at the same time
the class of situations where the speedup would happen.

Excluding pal8 (I am not aware of any usage case for it, though I do not
pretend to know for all the ffmpeg/cinepak users) and yuv420p (hardware
scalers in X11 like it but they are in practice slower than cinepak :)
would remove about half the new code. Further removing rgb24 would
reduce it even more.

But neither me nor you are qualified to estimate the usefulness
of the formats you/me do not use ourselves.

I am qualified to state the need for _fast_ decoding, for the moment using
rgb565 in the first hand and rgba in some other situations. I expect that
the need for the other formats can become vital anytime when hardware
changes. That's another reason why even the formats without a _known_
use have a value and my motivation for having implemented them.

OTOH regarding your negative stance to the patch,

you did not come back when I kindly asked you to comment on these two
fully opposite opinions of yours about the handling of cinepak in ffmeg:

-
https://ffmpeg.org/pipermail/ffmpeg-devel/2013-February/138811.html
Paul B Mahol onemda at gmail.com
Sun Feb 10 19:34:53 CET 2013

> i think hes asking if it should be a new colorspace in swscale, or
> added into the cinepak decoder/encoder common code.   
> 
> by your answer i think it has to be a new swscale colorspace? 

Please leave libswscale alone.

-
https://ffmpeg.org/pipermail/ffmpeg-devel/2017-February/207134.html
Paul B Mahol onemda at gmail.com
Tue Feb 14 11:03:41 EET 2017

Correct way in solving this is outputing in cinepak decoder actual
native format that it
uses and not do any conversions of colorspace which is currently done.
Implement correct colorspace conversions of cinepak format to others
in libswscale.

-

This gives me a reasonable ground to assume that your stance can change.

Friendly yours,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

Hello,

Here comes the latest version of the patch, with adjustments
made according to all substantial feedback comments.

Among others the code has been further deduplicated to ease maintenance.

Hopefully the 2-4 times improvement of the decoding speed justifies the
growth of the affected source file (from 491 to 979 lines) and also the
projected added maintenance complexity of about 15 extra lines to edit
once in 4 years (derived from the change history of this source file).

Note that there is no alternative codec+decoder combination capable of
achieving a comparable speed. In this respect there is no substitute
at all. None. There is no chance to come near this performance via
swscaler either. This has been explained in detail.

It is up to the persons taking decisions on behalf of the project
to accept or reject this offer.

If the policies or personal tastes hinder such an improvement,
then this not a problem for me, but possibly for the project :)

Regards,
Rune
>From 24c1bbc11b1f8d806fd5550f1c6f71a68c564f44 Mon Sep 17 00:00:00 2001
From: Rl 
Date: Sat, 25 Feb 2017 18:31:28 +0100
Subject: [PATCH 1/2] Cinepak decoding: speed up several-fold by supporting
 multiple output pixel formats.

Optimized decoding to rgb24 and pal8.
Added rgb32, rgb565, yuv420p, each faster than to rgb24.

Not counting any format conversions:speedup 3-12%

Counting the impact of format conversion
with the best non-dithering conversion quality: speedup 2-4 times

 an example, generating rgb565
 (including overheads, underestimation of the actual decoding speedup)
 
 matrixbench_mpeg2.mpg (720x567) encoded with ffmpeg into Cinepak
 using default settings, decoding on an i5 3570K, 3.4 GHz:
 ...
 fast_bilinear:  ~65x realtime
 patch w/rgb565 override:~154x realtime
 
 https://ffmpeg.org/pipermail/ffmpeg-devel/2017-February/206799.html

Palettized input can be decoded to any of the output formats,
pal8 output is still limited to palettized input.

With input other than palettized/grayscale
yuv420p is approximated by the Cinepak colorspace.

The output format can be chosen at runtime
by an ffmpeg command line option or otherwise
via the API (get_format() callback).

The default output format is unchanged: rgb24.
---
 libavcodec/cinepak.c | 798 +--
 1 file changed, 643 insertions(+), 155 deletions(-)

diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c
index d657e9c0c1..97836b3ab1 100644
--- a/libavcodec/cinepak.c
+++ b/libavcodec/cinepak.c
@@ -31,6 +31,8 @@
  *
  * Cinepak colorspace support (c) 2013 Rl, Aetey Global Technologies AB
  * @author Cinepak colorspace, Rl, Aetey Global Technologies AB
+ * Extra output formats / optimizations (c) 2017 Rl, Aetey Global Technologies 
AB
+ * @author Extra output formats / optimizations, Rl, Aetey Global Technologies 
AB
  */
 
 #include 
@@ -39,23 +41,49 @@
 
 #include "libavutil/common.h"
 #include "libavutil/intreadwrite.h"
+#include "libavutil/opt.h"
+#include "libavutil/pixdesc.h"
 #include "avcodec.h"
 #include "internal.h"
 
+/* rounding to nearest; truncation would be slightly faster
+ * but it noticeably affects the picture quality;
+ * unless we become extremely desperate to use every single cycle
+ * we do not bother implementing a choice -- rl */
+#define PACK_RGB_RGB565(r,g,b) 
(((av_clip_uint8((r)+4)>>3)<<11)|((av_clip_uint8((g)+2)>>2)<<5)|(av_clip_uint8((b)+4)>>3))
 
-typedef uint8_t cvid_codebook[12];
+/*
+ * more "desperate/ultimate" optimization possibilites:
+ * - possibly (hardly?) spare a cycle or two by not ensuring to stay
+ *   inside the frame at vector decoding (the frame is allocated with
+ *   a margin for us as an extra precaution, we can as well use this)
+ * - skip filling in opacity when it is not needed by the data consumer,
+ *   in many cases rgb32 is almost as fast as rgb565, with full quality,
+ *   improving its speed can make sense
+ * - add "fast rgb565" with truncation instead of rounding
+ */
+
+typedef union cvid_codebook {
+uint32_t   rgb32[256][ 4];
+uint8_trgb24[256][12];
+uint16_t  rgb565[256][ 4];
+uint8_t  yuv420p[256][ 6];
+uint8_t pal8[256][ 4];
+} cvid_codebook;
 
-#define MAX_STRIPS  32
+#define MAX_STRIPS  32/* an arbitrary limit -- rl */
 
 typedef struct cvid_strip {
 uint16_t  id;
 uint16_t  x1, y1;
 uint16_t  x2, y2;
-cvid_codebook v4_codebook[256];
-cvid_codebook v1_codebook[256];
+cvid_codebook v4_codebook;
+cvid_codebook v1_codebook;
 } cvid_strip;
 
-typedef struct CinepakContext {
+typedef struct CinepakContext CinepakContext;
+struct CinepakContext {
+const AVClass *class;
 
 AVCodecContext *avctx;
 AVFrame *frame;
@@ -71,57 +99,259 @@ typedef struct CinepakContext {
 int sega_film_skip_bytes;
 
 uint32_t pal[256];
-} CinepakContext;
 
-static void cinepak_decode_codebook 

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

[u-9iep]

Support for conditional compilation,
a simple non-intrusive means to avoid binary bloat when this is relevant.

OTOH this change adds some extra lines to the source, which
may be acceptable or not.

Regards,
Rune
>From 11b5906e5161748b77bbad242ac28a49851c8b4f Mon Sep 17 00:00:00 2001
From: Rl 
Date: Sat, 25 Feb 2017 19:14:18 +0100
Subject: [PATCH 2/2] Cinepak decoding: allow selective build of support for
 output pixel formats.

A simple non-intrusive means to avoid bloat
if the library is being built for some specific scenario.
---
 libavcodec/cinepak.c | 62 
 1 file changed, 62 insertions(+)

diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c
index 97836b3ab1..e32b07ce8a 100644
--- a/libavcodec/cinepak.c
+++ b/libavcodec/cinepak.c
@@ -46,6 +46,20 @@
 #include "avcodec.h"
 #include "internal.h"
 
+/* allow to choose which output formats are to be supported,
+ * if nothing specific is enabled explicitly, activate all */
+#if !defined(CINEPAK_ENABLE_DECODE_TO_RGB32) && \
+!defined(CINEPAK_ENABLE_DECODE_TO_RGB24) && \
+!defined(CINEPAK_ENABLE_DECODE_TO_RGB565) && \
+!defined(CINEPAK_ENABLE_DECODE_TO_YUV420P) && \
+!defined(CINEPAK_ENABLE_DECODE_TO_PAL8)
+#define CINEPAK_ENABLE_DECODE_TO_RGB32
+#define CINEPAK_ENABLE_DECODE_TO_RGB24
+#define CINEPAK_ENABLE_DECODE_TO_RGB565
+#define CINEPAK_ENABLE_DECODE_TO_YUV420P
+#define CINEPAK_ENABLE_DECODE_TO_PAL8
+#endif
+
 /* rounding to nearest; truncation would be slightly faster
  * but it noticeably affects the picture quality;
  * unless we become extremely desperate to use every single cycle
@@ -64,11 +78,21 @@
  */
 
 typedef union cvid_codebook {
+#ifdef CINEPAK_ENABLE_DECODE_TO_RGB32
 uint32_t   rgb32[256][ 4];
+#endif
+#ifdef CINEPAK_ENABLE_DECODE_TO_RGB24
 uint8_trgb24[256][12];
+#endif
+#ifdef CINEPAK_ENABLE_DECODE_TO_RGB565
 uint16_t  rgb565[256][ 4];
+#endif
+#ifdef CINEPAK_ENABLE_DECODE_TO_YUV420P
 uint8_t  yuv420p[256][ 6];
+#endif
+#ifdef CINEPAK_ENABLE_DECODE_TO_PAL8
 uint8_t pal8[256][ 4];
+#endif
 } cvid_codebook;
 
 #define MAX_STRIPS  32/* an arbitrary limit -- rl */
@@ -233,6 +257,7 @@ static int cinepak_decode_vectors_##pixel_format 
(CinepakContext *s, cvid_strip
  * (instead of in-loop checking) */\
 VECTOR_STREAM_PARSING\
 
+#ifdef CINEPAK_ENABLE_DECODE_TO_RGB32
 DECODE_CODEBOOK(rgb32)
 uint32_t *p = codebook->rgb32[0];
 
@@ -320,7 +345,9 @@ DECODE_VECTORS(rgb32)
 
 return 0;
 }
+#endif /* #ifdef CINEPAK_ENABLE_DECODE_TO_RGB32 */
 
+#ifdef CINEPAK_ENABLE_DECODE_TO_RGB24
 DECODE_CODEBOOK(rgb24)
 uint8_t *p = codebook->rgb24[0];
 
@@ -411,7 +438,9 @@ DECODE_VECTORS(rgb24)
 
 return 0;
 }
+#endif /* #ifdef CINEPAK_ENABLE_DECODE_TO_RGB24 */
 
+#ifdef CINEPAK_ENABLE_DECODE_TO_RGB565
 DECODE_CODEBOOK(rgb565)
 uint16_t *p = codebook->rgb565[0];
 
@@ -498,7 +527,9 @@ DECODE_VECTORS(rgb565)
 
 return 0;
 }
+#endif /* #ifdef CINEPAK_ENABLE_DECODE_TO_RGB565 */
 
+#ifdef CINEPAK_ENABLE_DECODE_TO_YUV420P
 /* a simplistic version to begin with, it is also fast -- rl */
 DECODE_CODEBOOK(yuv420p)
 uint8_t *p = codebook->yuv420p[0];
@@ -620,7 +651,9 @@ DECODE_VECTORS(yuv420p)
 
 return 0;
 }
+#endif /* #ifdef CINEPAK_ENABLE_DECODE_TO_YUV420P */
 
+#ifdef CINEPAK_ENABLE_DECODE_TO_PAL8
 /* here we do not expect anything besides palettized video,
  * nor check the data for validity, which should be ok,
  * to the best of our knowledge we do not write beyond the bounds */
@@ -702,6 +735,7 @@ DECODE_VECTORS(pal8)
 
 return 0;
 }
+#endif /* #ifdef CINEPAK_ENABLE_DECODE_TO_PAL8 */
 
 static int cinepak_decode_strip (CinepakContext *s,
  cvid_strip *strip, const uint8_t *data, int 
size)
@@ -844,20 +878,38 @@ static int cinepak_decode (CinepakContext *s)
 
 /* given a palettized input */
 static const enum AVPixelFormat pixfmt_list[] = {
+#ifdef CINEPAK_ENABLE_DECODE_TO_RGB24
 AV_PIX_FMT_RGB24,
+#endif
+#ifdef CINEPAK_ENABLE_DECODE_TO_RGB32
 AV_PIX_FMT_RGB32,
+#endif
+#ifdef CINEPAK_ENABLE_DECODE_TO_RGB565
 AV_PIX_FMT_RGB565,
+#endif
+#ifdef CINEPAK_ENABLE_DECODE_TO_YUV420P
 AV_PIX_FMT_YUV420P,
+#endif
+#ifdef CINEPAK_ENABLE_DECODE_TO_PAL8
 AV_PIX_FMT_PAL8, /* only when input is palettized */
+#endif
 AV_PIX_FMT_NONE
 };
 
 /* given a non-palettized input */
 static const enum AVPixelFormat pixfmt_list_2[] = {
+#ifdef CINEPAK_ENABLE_DECODE_TO_RGB24
 AV_PIX_FMT_RGB24,
+#endif
+#ifdef CINEPAK_ENABLE_DECODE_TO_RGB32
 AV_PIX_FMT_RGB32,
+#endif
+#ifdef CINEPAK_ENABLE_DECODE_TO_RGB565
 AV_PIX_FMT_RGB565,
+#endif
+#ifdef CINEPAK_ENABLE_DECODE_TO_YUV420P
 AV_PIX_FMT_YUV420P,
+#endif
 AV_PIX_FMT_NONE
 };
 
@@ -891,16 +943,26 @@ static av_cold int cinepak_decode_init(AVCodecContext 
*avctx)
  break;\
 
 switch (avctx->pix_fmt) {
+#ifdef CINEPAK_ENABLE_DECODE_TO_RGB32
 case AV_PIX_FMT_RGB32:   DECODE_TO(rgb32)

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

[u-9iep]

Hi Ronald,

On Tue, Feb 14, 2017 at 09:46:55AM -0500, Ronald S. Bultje wrote:
> > The huge difference in the amount of the data to be processed; in other
> > words the very essence of the vector quantization technology where frame
> > data is represented by a codebook, by design meant to be much smaller.
> 
> 
> We acknowledge that. We understand this. Nobody disputes this.

Just several lines below you assume (why?) that using this specific advantage
in Cinepak would create any ground for a "corresponding change" in h264.
As far as I know h264 does not use vector quantization on the final
output data, or what do you mean by the following:

> But we still don't think breaking the modularization is the right way
> forward. I'm sorry. We're thinking about this in terms of maintainability
> as well as speed. The problem is that once we allow this, people will ask
> for 16bit output in h264 for all native bitdepths, or even packed formats
> (Kieran already asked).

Unfortunately I do not see this as having any real relevance.
If there indeed is a gain to be collected for h264, it should be weighed
against the cost to be caused by a change _there_.

As a matter of fact, I do not suggest "breaking modularity in ffmpeg".

Modularity, a very useful concept, like any other concept has its area
of usefulness. An approach very reasonable in most situations should
not be mistaken for "the best one in all cases".

Here we have a case where enforced application of this generally useful
concept is remarkably far from optimal.

> We know it's faster. We also know it's unreasonable from a maintenance

You do not know the latter.
Your guess possibly reflects the feeling of a fundamental concept
being neglected, but this in not a case of neglection/ignorance.

> perspective. We have to draw a line somewhere. It's an imperfect line but
> there's a reason for it. I'm sorry. That's life.

You do not have to be sorry, just be substantial.

I have checked the changes done to Cinepak decoder during the 4 years
since it began to decode correctly.

If the change being discussed now would have been applied back then,
one of the later commits would have had about 15 extra lines to change
(btw, in a very regular fashion: "frame." => "frame->" nothing else).

Another commit would have to change 5 lines instead of 1.

The latter indicated indeed an unnecessary duplication in variable
declarations, sorry for that. I now change the patch to avoid this.

That's all. Tell me if I missed something.

The decoder is used rarely but it is indispensable when maximal speed
is needed. There is no substitute.

Is a 3-fold improvement in the decoding speed worth 15 extra lines
to change once in 4 years?

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

On Tue, Feb 14, 2017 at 10:03:41AM +0100, Paul B Mahol wrote:
> Correct way in solving this is outputing in cinepak decoder actual
> native format that it
> uses and not do any conversions of colorspace which is currently done.
> Implement correct colorspace conversions of cinepak format to others
> in libswscale.

Would you comment on what is the difference between now and the situation

 https://ffmpeg.org/pipermail/ffmpeg-devel/2013-February/138811.html

when you (?) wrote on the same topic

---
> i think hes asking if it should be a new colorspace in swscale, or
> added into the cinepak decoder/encoder common code.
>
> by your answer i think it has to be a new swscale colorspace?

Please leave libswscale alone.
---

That position was IMHO reasonable.

Commenting on your current suggestion:

What would be the advantage of moving the Cinepak-specific conversions
into the general purpose library? Either this would mean as many
variations of the conversion code as there are in the decoder now or
the conversion would be done through some intermediate format, which is
a loss per se. The code still only would be used by Cinepak.

Besides this, for a purely technical reason the conversions done in
Cinepak are not suitable for delayed delegation to swscaler.

The reason has been already pointed out in the thread:

The huge difference in the amount of the data to be processed; in other
words the very essence of the vector quantization technology where frame
data is represented by a codebook, by design meant to be much smaller.

(BTW mathematically DCT to the contrary is equivalent to referencing an
extremely large codebook. This codebook does not have to be transmitted
or stored, neither of which would be practical. Instead its entries are
generated on-demand, for each frame data block. This is the situation
where swscale is an efficient tool.)

Hope this helps.

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

On Tue, Feb 14, 2017 at 06:51:46AM +0100, wm4 wrote:
> On Mon, 13 Feb 2017 18:51:39 +0100
> u-9...@aetey.se wrote:
> 
> > Then abstracting a "mini-swscale" could become justifiable.
> 
> And this is why we should probably reject this patch.
> What you wrote paints a horrifying future.
--^
> 
> Note that we would have this discussion even if it'd speed up the h264
> decoder. Pissing all over modularization is not a good thing to do.
---^^^
> 
> If you really want to get anything applied, you should probably try
> looking at outputting ycgco, which appears to be the native colorspace
> of the codec (and convert it vf_colorspace, I guess).

Dear wm4,

Your readiness to give feedback and your endeavor to keep the high quality
of the code are appreciated. Nevertheless:

I kindly ask you to mind your use of disparaging terms
(emotionally charged expressions like "horrifying" or "pissing"
which attribute a negative quality or attitude to your opponent),
the corresponding phrases are marked above for your reference

and please check your data.

For additional information I would suggest

 https://ffmpeg.org/developer.html#Code-of-conduct

 https://multimedia.cx/mirror/cinepak.txt

 the contents of this thread

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

Thanks Michael,

Your corrections are appreciated.

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

> > +/* #include "libavutil/avassert.h" */
> 
> useless commented out code

I hesitated but left it together with the corresponding commented out
assert() statement to serve as an indication of the validity assumption
we make for pal8. Will change.

> > +av_log(avctx, AV_LOG_ERROR, "Unsupported pixel format %d\n", 
> > avctx->pix_fmt);
> 
> av_get_pix_fmt_name()

Thanks.

> > @@ -488,4 +1026,6 @@ AVCodec ff_cinepak_decoder = {
> >  .close  = cinepak_decode_end,
> >  .decode = cinepak_decode_frame,
> >  .capabilities   = AV_CODEC_CAP_DR1,
> > +.pix_fmts   = pixfmt_list,
> 
> This is possibly unneeded

ok!

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

Hello,

This is my best effort attempt to make the patch acceptable
by the upstream's criteria.

Daniel, do you mind that I referred to your message in the commit?
I believe is is best to indicate numbers from a third party measurement.

The code seems to be equvalent to the previous patch,
with about 20% less LOC.

This hurts readability (my subjective impression) but on the positive side
the change makes the structure of the code more explicit.

Attaching the patch.

Now I have done what I can, have to leave.
Unless there are bugs there in the patch, my attempt to contribute ends
at this point.

Thanks to everyone who cared to objectively discuss a specific case
of ffmpeg usage, the implications of techniques around VQ and whether/why
some non-traditional approaches can make sense.

Good luck to the ffmpeg project, it is very useful and valuable.

Best regards,
Rune
>From 0c9badec5d144b995c0bb52c7a80939b672be3f5 Mon Sep 17 00:00:00 2001
From: Rl 
Date: Sat, 11 Feb 2017 20:28:54 +0100
Subject: [PATCH] Cinepak: speed up decoding several-fold, depending on the
 scenario, by supporting multiple output pixel formats.

Decoding to rgb24 and pal8 is optimized.

Added rgb32, rgb565, yuv420p, each with faster decoding than to rgb24.

The most noticeable gain is achieved by the created possibility
to skip format conversions, for example when decoding to rgb565

Using matrixbench_mpeg2.mpg (720x567) encoded with ffmpeg into Cinepak
using default settings, decoding on an i5 3570K, 3.4 GHz:

bicubic (default):  ~24x realtime
fast_bilinear:  ~65x realtime
patch w/rgb565 override:~154x realtime

(https://ffmpeg.org/pipermail/ffmpeg-devel/2017-February/206799.html)

palettized input can be decoded to any of the output formats,
pal8 output is still limited to palettized input

with input other than palettized/grayscale
yuv420 is approximated by the Cinepak colorspace

The output format can be chosen at runtime by an option or via the API.
---
 libavcodec/cinepak.c | 844 +--
 1 file changed, 692 insertions(+), 152 deletions(-)

diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c
index d657e9c0c1..7b08e20e06 100644
--- a/libavcodec/cinepak.c
+++ b/libavcodec/cinepak.c
@@ -31,6 +31,8 @@
  *
  * Cinepak colorspace support (c) 2013 Rl, Aetey Global Technologies AB
  * @author Cinepak colorspace, Rl, Aetey Global Technologies AB
+ * Extra output formats / optimizations (c) 2017 Rl, Aetey Global Technologies 
AB
+ * @author Extra output formats / optimizations, Rl, Aetey Global Technologies 
AB
  */
 
 #include 
@@ -39,23 +41,48 @@
 
 #include "libavutil/common.h"
 #include "libavutil/intreadwrite.h"
+#include "libavutil/opt.h"
+/* #include "libavutil/avassert.h" */
 #include "avcodec.h"
 #include "internal.h"
 
+/* rounding to nearest; truncation would be slightly faster
+ * but it noticeably affects the picture quality;
+ * unless we become extremely desperate to use every single cycle
+ * we do not bother implementing a choice -- rl */
+#define PACK_RGB_RGB565(r,g,b) 
(((av_clip_uint8((r)+4)>>3)<<11)|((av_clip_uint8((g)+2)>>2)<<5)|(av_clip_uint8((b)+4)>>3))
 
-typedef uint8_t cvid_codebook[12];
+/*
+ * more "desperate/ultimate" optimization possibilites:
+ * - possibly (hardly?) spare a cycle or two by not ensuring to stay
+ *   inside the frame at vector decoding (the frame is allocated with
+ *   a margin for us as an extra precaution, we can as well use this)
+ * - skip filling in opacity when it is not needed by the data consumer,
+ *   in many cases rgb32 is almost as fast as rgb565, with full quality,
+ *   improving its speed can make sense
+ */
+
+typedef union cvid_codebook {
+uint32_t  rgb32[256][ 4];
+uint8_t   rgb24[256][12];
+uint16_t rgb565[256][ 4];
+uint8_t  yuv420[256][ 6];
+uint8_tpal8[256][ 4];
+} cvid_codebook;
 
-#define MAX_STRIPS  32
+#define MAX_STRIPS  32/* an arbitrary limit -- rl */
 
 typedef struct cvid_strip {
 uint16_t  id;
 uint16_t  x1, y1;
 uint16_t  x2, y2;
-cvid_codebook v4_codebook[256];
-cvid_codebook v1_codebook[256];
+cvid_codebook v4_codebook;
+cvid_codebook v1_codebook;
 } cvid_strip;
 
-typedef struct CinepakContext {
+typedef struct CinepakContext CinepakContext;
+struct CinepakContext {
+const AVClass *class;
 
 AVCodecContext *avctx;
 AVFrame *frame;
@@ -71,57 +98,192 @@ typedef struct CinepakContext {
 int sega_film_skip_bytes;
 
 uint32_t pal[256];
-} CinepakContext;
 
-static void cinepak_decode_codebook (cvid_codebook *codebook,
- int chunk_id, int size, const uint8_t 
*data)
-{
-const uint8_t *eod = (data + size);
-uint32_t flag, mask;
-int  i, n;
-uint8_t *p;
-
-/* check if this chunk contains 4- or 6-element vectors */
-n= (chunk_id & 0x04) ? 4 : 6;
-flag = 0;
-

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

[u-9iep]

On Wed, Feb 08, 2017 at 09:02:44AM -0500, Ronald S. Bultje wrote:
> I encourage you to fork the code and publish the faster decoder so others
> with use cases like yours can use it. It's free software, you're allowed
> and encouraged to do that.

I considered this possibility. My scope of contribution is though "a
useful modification". Leading/maintaining a multimedia-tool-project is
a very different thing and not something I am interested in.

> I just don't think it belongs in upstream FFmpeg.

This depends of course on the idea of what FFmpeg is.
I am not qualified to make such a definition, given my limited
engagement here.

My expectation was "the Swiss Army Knife of multimedia" and "the most
efficient tool in its class". If either of those is not or no longer
relevant, not my thing to decide, then the patch is indeed wrongly
addressed.

> (Going back to the native format discussion, I looked at decode_codebook
> and it appears the native format is actually 8-bit/component YCgCo; it's

Not exactly, it is similar but Cinepak-specific, it is also optimized for
conversion to rgb.

> interesting that we chose rgb24 as output format, I'm guessing it's because

It is the natural target format of Cinepak's design.

> YCgCo support in FFmpeg is non-existent.)

Kind of.
(https://ffmpeg.org/pipermail/ffmpeg-devel/2013-February/138811.html)

This was fortunate, allowing to notice the gain associated with filling
the codebook with the converted data, compared to doing the conversion
on the resulting frame.

From the commit message Mon Feb 18 18:00:33 2013 +0100
"
old decoder + conversion to rgb: fps = 2618
old decoder, without converting to rgb:  fps = 4012

new decoder, producing rgb:  fps = 4502
"

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

On Tue, Feb 07, 2017 at 09:07:02PM -0700, Daniel Verkamp wrote:
> There is already a rgb24-to-rgb565 path, but it does not get used by
> default because of the needsDither check in ff_get_unscaled_swscale().
> If the scaling algorithm is set to fast_bilinear, needsDither is
> ignored and the RGB24 to RGB16 converter is used. (In either case, no
> actual scaling is performed, so the scaling algorithm is irrelevant
> aside from selecting dithering.) Looking at the mplayer docs, this is
> probably equivalent to '-sws 0'.
> 
> e.g.
>   ffmpeg -i  -f null -c rawvideo -pix_fmt rgb565le -sws_flags
> fast_bilinear /dev/null
> 
> Using matrixbench_mpeg2.mpg (720x567) encoded with ffmpeg into Cinepak
> using default settings, decoding on an i5 3570K, 3.4 GHz:
> 
> bicubic (default):  ~24x realtime
> fast_bilinear:  ~65x realtime
> patch w/rgb565 override:~154x realtime
> 
> As far as I can tell, this patch doesn't do any dithering for RGB565,
> so the fast_bilinear (non-dithering) swscale converter is a fair
> comparison.

Thanks Daniel.

Yes, the patch does accurate rounding, no dithering.

Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/3] libavcodec/cinepakenc.c: comments cleanup (contents)

[u-9iep]

On Tue, Feb 07, 2017 at 10:50:42PM +0100, Michael Niedermayer wrote:
> There is no "Rl" in there
> 
> git removes that when applying with git am
> git am
> ...
> From: Rl 
> ...
> git show
> ...
> Author: addr-see-the-webs...@aetey.se 

Oh! :-/

> I dont know why git does that but it does, i retested this as it seemed
> rather unexpected and odd

Yes, this is odd.

> I can manually override this and force git to use Rl as name so i
> will do that, but this may be error prone if git fails to keep Rl
> even after there are commits in the tree with that name.

How much does it check the validity of the part inside <> ?
What about  ?
Currently this should not be possible to confuse with a valid mail
address.

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

Dear Henrik,

On Tue, Feb 07, 2017 at 07:21:39PM +0100, Hendrik Leppkes wrote:
> > Why not, if there will be a data consumer with this hypothetical format
> > and we will need speed? Why not? This _is_ efficient at the decoder,
> > it is just many of the developers ignored this fact until now.
> 
> If you don't understand why this is bad, then trying to explain it
> again is just wasted time.

Unfortunately this is my very impression, hard to explain "evident"
things because the other party is blinded by his/her own perspective.

> I'll give you a hint: What if every codec would do this? Surely that
> would be faster, but noone in their right mind would ever want to work
> on such an abonimation.

Let me return another hint :)
not every codec (but a small minority) is actually suited for such
optimizations, so your imagined hellish world would not be much faster.

Abomination or not is also a matter of personal taste. I do not feel that
Cinepak code (current one, which since long ago does format conversions :)
is more repulsive than the code of *264 :) but this is irrelevant!

What is relevant is how well they work in the long run.

> >> not a decoders job to convert from RGB to YUV.
> >
> > What is the criteria to choose where the job is to be done?
> > My point is efficiency. What is yours?
> 
> If you want effieciency above everything else, maybe you should write
> a very specific application tailored for your one specific use-case,

This is an example of being "blinded" by one's perspective.

You are positively/constructively trying to put yourself in my situation
and suggest a good solution, but this is highly unreliable. In this
particular case: I do not generally have the power over the applications.
Ther is also the plural "s" in applications.

> and not use a generic multimedia frame work like ffmpeg.

This would not help me a tiny bit. Ffmpeg is a very useful tool,
not being able to use it would be devastating! :) You should be proud.

> Anyway, you don't seem to be understanding our points at all, so the
> chances of this ever being commited are reaching zero.

Doing my best. :)

Unfortunately your position looks like "I know what is a right
solution. If somebody does not agree, s/he is not understanding".

This is because people (you and me included) tend to underestimate:

the invisible but huge limitations which our environment and perspective
put on our capacity to see each other's situation, needs, constraints
and criteria.

You assume a lot about why I am doing thing a certain way.
Many of your assumptions are unfounded.

I assume a lot about what are the developer's motivations and priorities.
Certainly many of these guesses are wrong.

Let's calm down and stop assuming at least one thing,
that the other party is stupid.

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

On Tue, Feb 07, 2017 at 12:54:23PM -0500, Ronald S. Bultje wrote:
> On Tue, Feb 7, 2017 at 12:04 PM,  wrote:
> 
> > cinepak, rgb2419.7 (via the fast bilinear swscaler)
> > cinepak, internal rgb565   6.0
> 
> 
> The reason that your decoder-integrated colorspace convertor is so much
> faster than swscale is because swscale is converting internally to yuv420p
> using a scaling filter, and then back to rgb565 using another scaling
> filter.

(If this indeed happens, this does not sound exactly efficient, nor
is a conversion to yuv420p lossless itself, IOW a shame?)

> This is "easily" solved by adding a direct (possibly
> x86-simd-accelerated) rgb24-to-rgb565 converter into
> libswscale/swscale_unscaled.c, which would likely have nearly identical
> performance to what you're seeing here. Possibly even faster, because
> you're allowing for simd optimizations.

This unfortunately can not come near an identical performance because
it would have to work on several times more data (frame vs codebook).

Besides that, there would be at least an extra copy operation over
each frame, even if the conversion itself would be indefinitely fast.

Generally:

I value layered design as much as you do, but it introduces limitations.

For comparison, an example from a different domain, but well known:
ZFS. It shortcut several "design layers" in the storage subsystem
which allowed a lot of improvement and did not render ZFS unmaintainable.

The shortcuts I add (not introduce, just add to the present ones) are
of exactly the same nature as a specialized converter in libswscale.
I just add them in a place where they are several times more efficient
(the amount of data to handle).

Cinepak is hardly going to make an impact similar to ZFS now :)
but it has in fact been very big before.

In certain aspects, by the original design, it is still superior to
virtualy anything else at hand. With the proposed optimization we get the
best out of its virtues. It would be a waste to ignore the possibility.

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

Hello Ronald,

On Tue, Feb 07, 2017 at 10:29:01AM -0500, Ronald S. Bultje wrote:
> So, right now, the decoder outputs pal8 vs. rgb24 depending on the internal
> format of the bitstream, and you're changing it to do conversion so it
> outputs a selectable output format directly, right? (And then there's some
> discussion over how to select the format.)

A very good summary.

The only detail to note is that format conversion has always been done
inside the Cinepak decoder, the change does not "introduce" it in any way.

What is done is basically splitting the format conversion function which
until now did all the three conversions (palettized->pal8; grayscale->rgb24;
"cinepak"->rgb24) into separate functions, as a result each function
becoming simpler, faster and making it easy to produce "any" desired
format.

> My personal opinion is that it's not worth it to do colorspace conversion
> of any sort, including palette resolution, in a decoder. I understand using
> swscale to do the conversion is slower, but cinepak is a fringe codec and a
> new haswell i7 isn't that expensive. (Code maintenance has a cost also.)

Your point about code maintenance cost basically boils down to Cinepak
not being considered worth improvement, because any change incurs a cost
here (among others, the time we spend on this discussion) and because
it is easy somewhere else in a foreign usage case to replace hardware
to more capable. The first part makes sense but the other is hardly solid.

One of the users spent quite a bit of effort on implementing the speedup,
this is at least some kind of indication of the usefulness.

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

On Tue, Feb 07, 2017 at 04:17:30PM +0100, Hendrik Leppkes wrote:
> On Tue, Feb 7, 2017 at 3:57 PM,   wrote:
> > Still, given the disapproval of the "code quality" without a tangible
> > criteria to follow, I can hardly take any accomodating steps, barring
> > the omission of the unused code - would this step be enough?
> 
> The code duplication is still enormous and makes the entire approach

Do you mean the about dozen lines which by the bitstream design are
doing exactly the same but are repeated almost literally in every of
the 10 finctions? This is the duplication I see, do you mean this or
something else?

> look rather questionable, and on top of that some built-in yuv
> conversion is not really appropriate. Packing into different RGB

Why not appropriate if it is useful?
Any other way to do equally fast decoding to YUV?

> formats is one thing, but actually converting to another color format
> entirely is quite something else. Whats next, handling all sorts of

You talk past me! What makes you accept the one but not the other? :)

> various yuv colorspaces and subsampling factors?

Why not, if there will be a data consumer with this hypothetical format
and we will need speed? Why not? This _is_ efficient at the decoder,
it is just many of the developers ignored this fact until now.

> So at the very least the YUV part should be dropped at this point, its
> not a decoders job to convert from RGB to YUV.

What is the criteria to choose where the job is to be done?
My point is efficiency. What is yours?

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

On Tue, Feb 07, 2017 at 04:23:50PM +0100, wm4 wrote:
> > Still, given the disapproval of the "code quality" without a tangible
> > criteria to follow, I can hardly take any accomodating steps, barring
> > the omission of the unused code - would this step be enough?
> 
> Bad:
> - dead code

Already slated to be removed, I wrote.

> - code duplication

Would you give me an estimation of how many lines of code are actually
duplicated. I believe you just see the superficial resemblance, not
the differences.

> - not using standard API mechanisms (get_format)

You have to take this back and look at the patch.

> - using unusual mechanisms that are normally not used in FFmpeg

This is the whole point of the improvement.
If doing unusual useful things is a bad style here, I am leaving :)

I do not believe you really insist on this point.

>   libraries or libraries in general (configuration via getenv)

Ehh, wasn't this the "dead code" you complained about above?
Let's strike away this point.

So the only remaining unsettled point is which lines of code are
duplicated / how they are to be refactored.

I wrote about this change not being exactly trivial. May be I am not
fit for this particular task? Give me a hand, show how to do refactoring
without impacting readability and speed?

Otherwise it would be a pity to throw away an improvement just
because the author has his/her limitations.

> It's not so complicated if you make an effort to try to understand.

Indeed.

Friendly yours,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

On Fri, Feb 03, 2017 at 11:42:03AM +0100, u-9...@aetey.se wrote:
> On Fri, Feb 03, 2017 at 11:14:28AM +0100, wm4 wrote:
> > > On a 16-bit-per-pixel output with a CPU-based decoder you will
> > > not find _any_ over 25% of Cinepak speed. Raw video can not compete
> > > either when indata delivery bandwidth si limited.

> > I can't take this seriously, but I won't go and try finding a better
> > codec just to make a point.

Of course any benchmark result makes sense only within a carefully
specified context. It was incautious of me to make a statement in
such a general way.

I have to admit that the number 25% was not measured but extrapolated
from a specific test I made three years ago.
It is impossible to repeat that test (the hardware is gone) and the video
data was a specific sample which may have incurred a bias.

That's why now I have done a new test, with 15 minutes taken from a
(single) random movie as the input, compressed with ffmpeg to mpeg1video
at quality 8 and to cinepak with default quality settings, which in this
case yielded comparable quality.

When I take the user part of the output of
"time mplayer -fps 1 -vo null -quiet -vf format=fmt=bgr16 -sws 0" [*]

I get the following numbers on two different computers with Intel CPUs
running 32-bit code:  (averaging/rounding after three runs)

mpeg1, yuv420p23.6 (via the fast bilinear swscaler)
cinepak, rgb2419.7 (via the fast bilinear swscaler)
cinepak, internal rgb565   6.0

mpeg1, yuv420p52.1 (via the fast bilinear swscaler)
cinepak, rgb2443.1 (via the fast bilinear swscaler)
cinepak, internal rgb565  10.3

This corresponds to mpeg1 being for rgb565 output about 4-5 times slower
than cinepak.

I doubt that there is any other applicable codec with a faster decoder.
Tell me if you find it, this will be appreciated.

The "high color" 15-bit VQA was near cinepak in speed but it is
unfortunately not practically usable.

Regards,
Rune

[*] Remarkably, mplayer, an application with tight ties to ffmpeg,
does not seem to choose the appropriate decoder output format itself,
despite the availability of get_format() functionality...
Would you tell me which video player does? or where the bug is?

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

On Mon, Feb 06, 2017 at 11:05:06PM +0100, Clément Bœsch wrote:
> On Mon, Feb 06, 2017 at 10:05:10AM +0100, u-9...@aetey.se wrote:
> [...]
> > > No, code quality is not outside the scope of your patch.
> > 
> > Code quality is a subjective matter.
> > 
> 
> I'm not going to fight with you

Appreciated.

> several developers consider your patch
> does not pass the quality requirements of the project. It's arbitrary,
> [... skipped ...], but that's the current policy of the project.

Well said.

> Changing that policy is outside the scope of this patch.

:)

> [...]
> > > The use of the environment variable is not tolerable, this is a blocker.
> > 
> > It is explicitly specified that it is _not_ being used,
> 
> Then please drop the dead code.

Ok, why not.

Still, given the disapproval of the "code quality" without a tangible
criteria to follow, I can hardly take any accomodating steps, barring
the omission of the unused code - would this step be enough?

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/3] libavcodec/cinepakenc.c: comments cleanup (contents)

[u-9iep]

Hello Michael,

On Tue, Feb 07, 2017 at 03:09:56AM +0100, Michael Niedermayer wrote:
> > What about the other patches from that series?
> 
> > The second one is purely cosmetic (looks useful to me but not much
> > of concern)
> 
> I have no oppinion about // vs /**/ comments but in the absence of
> a reason to change them my gut feeling leans toward leaving things
> as they are for "git blame" simplification.

No objection.

> > while the third (tiny) one is about correcting an inverted
> > explanation in a comment.
> 
> I think the 3rd depends on the 2nd

No, the third is standalone.

It was possibly wrong to put them in the same series, but otherwise a
confusion would be probably easy, too.

> another point which was brought up by jb on IRC and in RL longer ago
> was that ideally the author field in git should contain a name.

> It may help with contacting one in the far future or with simply
> refering to the author as in

> who wrote the cinepack optimizations?
> Author: addr-see-the-webs...@aetey.se 

Contact data is not identity even though this fact is ignored in some
widely spread (misled) practices, which the above example illustrates
well. There are actually two separate questions:

Q: Who wrote the cinepak optimizations?
A: A person known as Rl @ Aetey Global Technologies AB.

Q: How to contact him/her?
A: The author does not wish to publicize any contact handle which
   is open to abuse, like a permanent mail address. YMMV.

> is a bit cumbersome but its not mine to judge that. If you want
> that in the author field, thats ok as far as iam concerned, i just
> have difficulty imagening that being intended over a pronouncable
> name (even if thats a pseudonym) before 

The data in the address field is not the/a pseudonym but a hint to a
possibly existing way to contact the author (no guarantee).

If you or any of the developers here wish to have a long term contact
handle to me, I will gladly provide it. But it will be invalidated as
soon as it becomes published or leaked.

Does this look reasonable to you?

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/3] libavcodec/cinepakenc.c: comments cleanup (contents)

[u-9iep]

On Thu, Feb 02, 2017 at 05:19:16PM +0100, Michael Niedermayer wrote:
> On Thu, Feb 02, 2017 at 04:22:28PM +0100, u-9...@aetey.se wrote:
> > On Thu, Feb 02, 2017 at 01:31:00PM +0100, Michael Niedermayer wrote:
> patch applied
> 
> thx

Thanks Michael.

What about the other patches from that series?

The second one is purely cosmetic (looks useful to me but not much
of concern) while the third (tiny) one is about correcting an inverted
explanation in a comment.

This inversion makes the logic of course noticeably harder to grasp,
IOW it is an apparent commenting bug, ought to be fixed.

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

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

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

Hello Mark,

On Sun, Feb 05, 2017 at 12:12:37PM +, Mark Thompson wrote:
> On 05/02/17 10:02, u-9...@aetey.se wrote:
> > To make it a bit more clear, my use case is
> > 
> > - various devices and videobuffers
> > - different applications which are not feasible to patch/teach/replace
> > 
> > Replacing ffmpeg with something else or modifying the applications 
> > unfortunately does not fit there, that's also why get_format() does not 
> > help.

> Even if you need to support such a use-case, doing it via the get_format() 
> callback is still the right thing to do.  Once you've done that, all normal 
> applications (including ffmpeg.c itself) can use the standard API as it 
> already exists to set the output format.  For your decoding-into-framebuffer 
> case the calling application must already be fully aware of the state of the 
> framebuffer (after all, it has to be able to make a suitable AVFrame to pass 
> to get_buffer2() so that you can avoid the extra copy), so adding 
> get_format() support to also communicate the format is not onerous.

Note that it is generally impossible to let the application decide
what to choose. Sometimes this may work, but the applications lack
the relevant needed knowledge, which is not guessable from
"what the layers before and after me report as supported formats
in a certain order".

> Then, if you have a proprietary application which cannot be modified because 
> you don't have the source, you could make a shim layer like:

Proprietary or not, I can hardly modify them.

A shim layer is certainly a possible workaround, but from my
perspective it would be "moving from a mildly inelegant
to a basically broken and unreliable technology".

The problem is

the technology of LD_*, an old and really bad design by itself.
Compared to a _library_specific_ envvar it is a long way father
from being usable as a general solution.
Note that an LD_* variable affects _linking_ (which is very intrusive)
for _all_ programs, related or not, in all child processes.
Note also that some applications do play tricks with LD_* on their own.
Have I said enough? :(


> static enum AVPixelFormat get_format_by_env_var(pix_fmt_list)
> {
> requested_pix_fmt = getenv(SOMETHING);
> if (requested_pix_fmt in pix_fmt_list)
> return requested_pix_fmt;
> else
> error;
> }

Exactly, but in my situation it is much more robust and easier to
enable the corresponding code in the decoder (or even add it there on
my own in the worst case) than play with binary patching on the fly,
which dynamic linking basically is.

So instead of forcing the possible fellow sysadmins in a similar situation
to patch, it would we nice to just let them build lilbavcodec with
this slightly non-standard (and pretty safe) behaviour.

> and LD_PRELOAD it into the application to achieve the same result (insofar as 
> that is possible - it seems unlikely that it will be able to use get_buffer() 
> appropriately, so there are probably going to be more redundant copies in the 
> application and you would need to patch it directly to eliminate them).

You see.

In any case, thanks for understanding the original problem.

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

On Fri, Feb 03, 2017 at 08:21:37PM +0100, wm4 wrote:
> With your special use-case (special as in does not fit into the API
> conventions of libavcodec), you might be better off with creating your
> own standalone cinepak decoder. That's not a bad thing; there's plenty
> of multimedia software that does not use libavcodec. Part of the reason
> is that one library can't make everyone happy and can't fit all
> use-cases.

To make it a bit more clear, my use case is

- various devices and videobuffers
- different applications which are not feasible to patch/teach/replace

Replacing ffmpeg with something else or modifying the applications 
unfortunately does not fit there, that's also why get_format() does not help.

Fortunately there is a solution for controlling the libraries out-of-band
even if it is not exactly elegant (I am more than well aware of implications
when using environment variables). If anything better would exist I'd use
it. This matter belongs otherwise to system- and deployment administraton,
offtopic to discuss here but you can safely take my word.

This does not have to be "supported" by ffmpeg but it is useful, not
harmful to have the corresponding code at hand, at least as a reference
(disabled as it is).

An improvement would be namespacing the environment variables to be
used together with ffmpeg libraries, but as long as such use is strongly
discouraged there is no possibility to allocate a namespace.

Otherwise, you see, ffmpeg fits very well for the needs in my environment,
it _does_ make people happy.

> Back to being "constructive" - the only way your code could get
> accepted is by implementing the get_format callback. There's a

Sure.

Indeed, activating get_format() can become useful for someone,
I should not have omitted it.

This is a tiny change.

I am now posting an amended and additionally commented patch.

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

On Fri, Feb 03, 2017 at 05:51:19PM +0100, Hendrik Leppkes wrote:
> On Fri, Feb 3, 2017 at 5:36 PM,   wrote:
> > So get_format() is not a solution, mo matter how good or misleading
> > its documentation is.
> 
> "The application" can implement the get_format callback anyway it
> wants, ask the user by carrier pigeon for all we care - but such user
> interaction does simply not belong into the avcodec library - hence we
> have the get_format callback for that, so that the decision can be
> moved outside of the codec library and into the calling user-code.
> Clearly whatever application you are working with should implement
> these choices, and you should not try to shoe-horn this into
> libavcodec, where it clearly does not belong.

You suggest I should shoe-horn this into every application.
Very helpful, thank you :)

As for "clearly", it is your personal feeling, not an argument.
Seriously.

> We do not want hacks around the established systems just because it
> doesn't fit your use-case or workflow, sorry.

You should listen more on those who actually live in their workflows.
It is there your code is being useful. Or not.

I happen to be in a suitable position (using the stuff and arranging it
for others) to estimate what is useful.

Based on this I am trying to help ffmpeg.
You are certainly free to refuse the help, for any reason :)

Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

Hello Ronald,

On Fri, Feb 03, 2017 at 08:52:53AM -0500, Ronald S. Bultje wrote:
> > I thought about generating the bodies of the functions from something
> > like a template but it did not feel like this would make the code more
> > understandable aka maintainable. So I wonder if there is any point in
> > doing this, given the same binary result.
> 
> wm4 has tried to make this point several times, it's about maintainability.

I really do understand.

> Let me explain, otherwise we'll keep going back and forth.

Thanks for the constructive discussion, hopefully I can make my point
clear here, and help you see the different perspective:

> Let's assume you have a function that does a, b and c, where b depends on
> the pix fmt. You're currently writing it as such:
> 
> function_565() {
> a
> b_565
> c
> }
> 
> function_24() {
> a
> b_24
> c
> }
> 
> function_32() {
> a
> b_32
> c
> }

Rather (or even more complicated) :

function_565() {
a; x; b; y; c; z; d;
}

function_24() {
a; p; b; q; c; r; d;
}

function_32() {
a; i; b; j; c; k; d;
}

Now, a small change in any of "a", "b", "c" would not necessarily have
the same consequence for all the functions, so templating would have
made it _harder_ to make safe changes, if we'd use something like

TEMPLATE(){ a; X; b; Y; c; Z; d; }

according to your suggestion.

Do you follow me? The "common" code happens to be the same _now_ but may
have to be different due to a change in any of a,b,c,d,x,y,z,p,q,r,i,j,k;

It would be also harder to follow the code flow.

> It should be pretty obvious that a and c are triplicated in this example.

Only in this statical situation, which is the contrary to maintainability.

> Now compare it with this:

[...]

> It might look larger, but that's merely because we're not writing out a and

It is not the size but the readability and change safety.

> Conclusion: better to maintain, identical performance. Only advantages, no
> disadvantages. Should be easy to accept, right?

Hope you see from the above: this is exactly why the code is structured
the way it does.

> So, what I don't
> understand then, is why below you're claiming that get_format() doesn't do
> this. this is exactly what get_format() does. Why do you believe
> get_format() isn't capable of helping you accomplish this?

get_format() apparently returns to the caller a suitable format
from the supplied list. I read the documentation as carefully as
I can and my interpretation is that it is the application who
is to define the function and that it is the ffmpeg framework which
supplies the list of formats supported by the codec. I appreciate
if somebody corrects me and/or improves the documentation.

But actually this does not matter in the particular situation.
None of the parties (the decoder, the framework, the application)
has all the necessary knowledge to be able to make the optimal choice.
It is the human operator/administrator who may know. The choice depends
among others e.g. on how fast swscaler is on the particular hardware
(use it or not?), how much is the contents sensitive to color
depths and so on.
How can get_format() help with answering these questions??).

So get_format() is not a solution, mo matter how good or misleading
its documentation is.

> > I did my best to look for a better way but it does not seem to be existing.

> Look into private options, for one. But really, get_format() solves this. I

We must have been thinking about different problems? The problem of
choosing the optimal format to decode to is not solvable with get_format()
(unless get_format() asks the human or e.g. checks an environment
variable or uses another out-of-band channel).

> can elaborate more if you really want me to, as above, but I feel you
> haven't really looked into it. I know this feeling, sometimes you've got
> something that works for you and you just want to commit it and be done
> with it.

:) I know this too, but I am quite confident in having done my homework
properly.

You try to explain your point and educate "the newcomer", which is very
helpful and appreciated.

Hope you are also prepared to see cases where your assumptions
do not hold.

> But that's not going to happen. The env variable will never be committed.
> Never. I guarantee it with 100% certainty. If you'd like this general

Then pity for ffmpeg, rejecting a useful feature without having any
comparable functionality.

Why not reserve a namespace in envvars, like
 FFMPEG_VIDEODEC_PIXFMT__
?
This would be (of course) much more general and useful than sticking
with the name I picked. Then some other codec could easily and coherently
take advantage of the same method as well.

There are possibly other (not necessarily pixel format related) scenarios
where corresponding namespaced envvars would help as out-of-band channels.

> feature to be picked up in the main codebase, you'll need to change at the
> very, very least how it is exposed as a selectable option. Env opts are not
> acceptable, they have 

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

[u-9iep]

On Fri, Feb 03, 2017 at 11:14:28AM +0100, wm4 wrote:
> > On a 16-bit-per-pixel output with a CPU-based decoder you will
> > not find _any_ over 25% of Cinepak speed. Raw video can not compete
> > either when indata delivery bandwidth si limited.
> > 
> > It has also an unused improvement margin in the encoder, still keeping
> > legacy decoders compatibility. The current encoder is already performing
> > a _way_ better than the proprietary one, so why leave this nice tool
> > unused where it can help?
> 
> I can't take this seriously, but I won't go and try finding a better
> codec just to make a point.

I take this as a statement that you believe something without having
the actual information.

[concerning get_format()]

> I don't know what you're thinking, but that's just wrong.

Thanks for making your point quite clear.

Have a nice day,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

On Thu, Feb 02, 2017 at 05:52:29PM +0100, wm4 wrote:
> On Thu, 2 Feb 2017 16:59:51 +0100
> u-9...@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

> > I would not call "twice" for "somewhat" :)
> 
> Well, your original mail mentioned only speedups up to 20%.

I have to recite from the original mail:
"
Avoiding frame pixel format conversion by generating rgb565 in the decoder
for a corresponsing video buffer yields in our tests (on MMX-capable
i*86) more than twice [sic] the playback speed compared to decoding to rgb24.
"

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

There is no guarantee that this is the same bug or that the same fix
would apply, because the functions are subtly different.

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

Cinepak is not a "game codec" but a solid video codec and has been used
very widely, for a long time, for a large range of applications.

For some niche applications it still provides the best result, being
a single and far away leader in decoding speed.

On a 16-bit-per-pixel output with a CPU-based decoder you will
not find _any_ over 25% of Cinepak speed. Raw video can not compete
either when indata delivery bandwidth si limited.

It has also an unused improvement margin in the encoder, still keeping
legacy decoders compatibility. The current encoder is already performing
a _way_ better than the proprietary one, so why leave this nice tool
unused where it can help?

> > Any suggestion which can replace this approach?
> 
> get_format would be more appropriate.

get_format() does not belong to the decoder, nor is it up to the task,
which I explained in a recent message.

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

I would be fine with commenting out these info-messages.

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

Hello Ronald,

On Thu, Feb 02, 2017 at 11:16:35AM -0500, Ronald S. Bultje wrote:
> On Thu, Feb 2, 2017 at 10:59 AM,  wrote:
> > 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.

> So, typically, we wouldn't duplicate the code, we'd template it. There's
> some examples in h264 how to do it. You'd have a single (av_always_inline)
> decode_codebook function, which takes "format" as an argument, and then
> have three av_noinline callers to it (using fmt=rgb565, fmt=rgb24 or
> fmt=rgb32).
> 
> That way performance works as you want it, without the source code
> duplication.

(Thanks for the pointer. I'll look at how it is done in h264, but: )

I thought about generating the bodies of the functions from something
like a template but it did not feel like this would make the code more
understandable aka maintainable. So I wonder if there is any point in
doing this, given the same binary result.

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

> Ah, yes, the question. So, the code change is quite big and it does various
> things, and each of these might have a better alternative or be good as-is.
> fundamentally, I don't really understand how _adding_ a colorspace
> conversion does any good to speed. It fundamentally always makes things
> slower. So can you explain why you need to _add_ a colorspace conversion?

It moves the conversion from after the decoder, where the data to convert
is all and whole frames, to the inside where the conversion applies
to the codebooks, by the codec design much less than the output.

> Why not just always output the native format? (And then do conversion in

The "native" Cinepak format is actually unknown to swscaler, and I
seriously doubt it would make sense to add it there, which would
just largely cut down the decoding efficiency.

> GPU if really needed, that is always near-free.)

You assume one have got a GPU.

The intention is to allow usable playback on as simple/slow devices
as possible.

> > > +char *out_fmt_override = getenv("CINEPAK_OUTPUT_FORMAT_OVERRIDE");
> >
> > > Absolutely not acceptable.
> >
> > 1. Why?
> >
> 
> Libavcodec is a library. Being sensitive to environment in a library, or
> worse yet, affecting the environment, is typically not what is expected.
> There are almost always better ways to do the same thing.

I did my best to look for a better way but it does not seem to be existing.

> For example, in this case:

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

> wm4 suggested earlier to implement a get_format() function. He meant this:
> 
> https://www.ffmpeg.org/doxygen/trunk/structAVCodecContext.html#ae85c5a0e81e9f97c063881148edc28b7

Unfortunately this is _not_ a solution.

I was of course aware of get_format(). Its functionality seems to
be intended for use by the applications: "decoding: Set by user".
Correct me if I am wrong (even better correct the documentation which
apparently is confusing?).

In any case, the codec has no means to know which output format is
"best" in a particular case so all it can do is to list the formats it
offers. This is done and using get_format() should work as expected,
which unfortunately does not solve anything:

There is of course hardly any application which tries to tune the codecs
to the output. (Mplayer seems to try but this does not seem to help,
possibly because its codec handling is generalized beyond ffmpeg codec
interfaces?)
Codecs efficiently supporting multiple formats are definitely a tiny
minority, so no surprize applications are not prepared to deal with this.

Applications also do lack any reliable knowledge of which format from the
codec is or is not efficient. It is only the deployment context which
can tell what is best for the particular case, which is known neither
to the codec nor to a general-purpose application.

To resum, I do not expect that there is any better solution than using
an out-of-band channel like a dedicated environment variable,
but would be of course happy to see one.

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

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

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/3] libavcodec/cinepakenc.c: comments cleanup (contents)

[u-9iep]

On Thu, Feb 02, 2017 at 01:31:00PM +0100, Michael Niedermayer wrote:
> > From: Rl 
> 
> Is it intended that your name is not in the Author field for git ?

This is coherent with my earlier patches (from 2014).

Yes.

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[u-9iep]

Hello,

On Sat, Jan 28, 2017 at 07:53:06PM +0100, Michael Niedermayer wrote:
> please provide a git compatible patch
> git format-patch / send-email

The corresponding patches (concerning comments in cinepak-related files)
have been resent in a git-compatible form 2017-01-29.
This patch applies after them.

It represents a large change in the Cinepak decoder for speed/efficiency.
Cinepak was always very good at speed, now it has become even (much) better.

Bought by the code size growth is a reduction of in-loop checking and
a capability to produce formats directly suitable for output devices,
without applying pixel format conversion afterwards to the whole frame.

Decoding to certain formats is also remarkably faster than to the default
rgb24, on i686
 to rgb32 seems to be about 15% faster than to rgb24
 to rgb565 seems to be over 20% faster than to rgb24

Besides this, on a slow device using a 16-bit depth provides a remarkable
speedup per se, compared to larger depths.

Avoiding frame pixel format conversion by generating rgb565 in the decoder
for a corresponsing video buffer yields in our tests (on MMX-capable
i*86) more than twice [sic] the playback speed compared to decoding to rgb24.

The default output format remains rgb24, with the full quality offered
by the codec.

Splitting this into multiple patches does not look very useful,
the changes make sense together.

Regards,
Rune
>From 300b8a4e712d1a404983b245aac501e09326ee72 Mon Sep 17 00:00:00 2001
From: Rl 
Date: Wed, 1 Feb 2017 19:44:40 +0100
Subject: [PATCH] Efficiently support several output pixel formats in Cinepak
 decoder

Optimize decoding in general and largely improve speed,
among others by the ability to produce device-native pixel formats.

The output format can be chosen at runtime by an option.

The format can be also chosen by setting environment variable
CINEPAK_OUTPUT_FORMAT_OVERRIDE, if this is enabled at build time.
---
 libavcodec/cinepak.c | 957 +--
 1 file changed, 845 insertions(+), 112 deletions(-)

diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c
index d657e9c0c1..76105fcc0c 100644
--- a/libavcodec/cinepak.c
+++ b/libavcodec/cinepak.c
@@ -31,6 +31,8 @@
  *
  * Cinepak colorspace support (c) 2013 Rl, Aetey Global Technologies AB
  * @author Cinepak colorspace, Rl, Aetey Global Technologies AB
+ * Extra output formats / optimizations (c) 2017 Rl, Aetey Global Technologies 
AB
+ * @author Extra output formats / optimizations, Rl, Aetey Global Technologies 
AB
  */
 
 #include 
@@ -39,23 +41,37 @@
 
 #include "libavutil/common.h"
 #include "libavutil/intreadwrite.h"
+#include "libavutil/opt.h"
+/* #include "libavutil/avassert.h" */
 #include "avcodec.h"
 #include "internal.h"
 
+/* rounding to nearest is quite cheap in my tests
+ * and yields a remarkable quality improvement
+ * compared to simple truncation -- rl */
+#define PACK_RGB_RGB565(r,g,b) 
(((av_clip_uint8((r)+4)>>3)<<11)|((av_clip_uint8((g)+2)>>2)<<5)|(av_clip_uint8((b)+4)>>3))
 
-typedef uint8_t cvid_codebook[12];
+typedef union cvid_codebook {
+uint32_t  rgb32[256][ 4];
+uint8_t   rgb24[256][12];
+uint16_t rgb565[256][ 4];
+uint8_t  yuv420[256][ 6];
+uint8_tpal8[256][ 4];
+} cvid_codebook;
 
-#define MAX_STRIPS  32
+#define MAX_STRIPS  32/* an arbitrary limit -- rl */
 
 typedef struct cvid_strip {
 uint16_t  id;
 uint16_t  x1, y1;
 uint16_t  x2, y2;
-cvid_codebook v4_codebook[256];
-cvid_codebook v1_codebook[256];
+cvid_codebook v4_codebook;
+cvid_codebook v1_codebook;
 } cvid_strip;
 
-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;
 
-static void cinepak_decode_codebook (cvid_codebook *codebook,
- int chunk_id, int size, const uint8_t 
*data)
+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);
+/* options */
+enum AVPixelFormat out_pixfmt;
+};
+
+#define OFFSET(x) offsetof(CinepakContext, x)
+#define VD AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_DECODING_PARAM
+static const AVOption options[] = {
+{"output_pixel_format", "set output pixel format: 
rgb24/rgb32/rgb565/yuv420p/pal8; yuv420p is approximate", OFFSET(out_pixfmt), 
AV_OPT_TYPE_PIXEL_FMT, {.i64=AV_PIX_FMT_RGB24}, -1, INT_MAX, VD },
+{ NULL },
+};
+
+static const AVClass cinepak_class = {
+.class_name = "cinepak decoder",
+.item_name  = 

[FFmpeg-devel] [PATCH 3/3] libavcodec/cinepak.c: fix a wrong (inverted) misleading comment

[u-9iep]

Attaching the new version.

Regards,
Rune
>From f2041c0aaa5209e5d52649f741b6ee1dbc7e9021 Mon Sep 17 00:00:00 2001
From: Rl 
Date: Sun, 29 Jan 2017 18:28:25 +0100
Subject: [PATCH 3/3] libavcodec/cinepak.c: fix a wrong (inverted) misleading
 comment

Make the comment message understandable and correct.
---
 libavcodec/cinepak.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c
index 737462bd9c..d657e9c0c1 100644
--- a/libavcodec/cinepak.c
+++ b/libavcodec/cinepak.c
@@ -155,8 +155,8 @@ static int cinepak_decode_vectors (CinepakContext *s, 
cvid_strip *strip,
 }
 }
 }
-/* to get the correct picture for not-multiple-of-4 cases let us fill
- * each block from the bottom up, thus possibly overwriting the top line
+/* to get the correct picture for not-multiple-of-4 cases let us fill each
+ * block from the bottom up, thus possibly overwriting the bottommost line
  * more than once but ending with the correct data in place
  * (instead of in-loop checking) */
 
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 2/3] libavcodec/cinepakenc.c: comments style cleanup

[u-9iep]

Attaching the new version.

Regards,
Rune
>From e6719743b78862f897133d3f69a5e88a6a9a803e Mon Sep 17 00:00:00 2001
From: Rl 
Date: Sun, 29 Jan 2017 18:14:19 +0100
Subject: [PATCH 2/3] libavcodec/cinepakenc.c: comments style cleanup

Avoid the present mixture of comment styles (C vs C++)
by converting to the traditional C style.
---
 libavcodec/cinepakenc.c | 340 +++-
 1 file changed, 192 insertions(+), 148 deletions(-)

diff --git a/libavcodec/cinepakenc.c b/libavcodec/cinepakenc.c
index a28f669070..8c27c2a1aa 100644
--- a/libavcodec/cinepakenc.c
+++ b/libavcodec/cinepakenc.c
@@ -80,22 +80,24 @@ OTHER DEALINGS IN THE SOFTWARE.
 #define STRIP_HEADER_SIZE 12
 #define CHUNK_HEADER_SIZE 4
 
-#define MB_SIZE 4   //4x4 MBs
+#define MB_SIZE 4   /* 4x4 MBs */
 #define MB_AREA (MB_SIZE*MB_SIZE)
 
-#define VECTOR_MAX 6//six or four entries per vector depending on 
format
-#define CODEBOOK_MAX 256//size of a codebook
+#define VECTOR_MAX 6/* six or four entries per vector depending on 
format */
+#define CODEBOOK_MAX 256/* size of a codebook */
 
-#define MAX_STRIPS  32  //Note: having fewer choices regarding the number 
of strips speeds up encoding (obviously)
-#define MIN_STRIPS  1   //Note: having more strips speeds up encoding the 
frame (this is less obvious)
-// MAX_STRIPS limits the maximum quality you can reach
-//when you want high quality on high resolutions,
-// MIN_STRIPS limits the minimum efficiently encodable bit rate
-//on low resolutions
-// the numbers are only used for brute force optimization for the first frame,
-// for the following frames they are adaptively readjusted
-// NOTE the decoder in ffmpeg has its own arbitrary limitation on the number
-// of strips, currently 32
+#define MAX_STRIPS  32  /* Note: having fewer choices regarding the number 
of strips speeds up encoding (obviously) */
+#define MIN_STRIPS  1   /* Note: having more strips speeds up encoding the 
frame (this is less obvious) */
+/*
+ * MAX_STRIPS limits the maximum quality you can reach
+ *when you want high quality on high resolutions,
+ * MIN_STRIPS limits the minimum efficiently encodable bit rate
+ *on low resolutions
+ * the numbers are only used for brute force optimization for the first frame,
+ * for the following frames they are adaptively readjusted
+ * NOTE the decoder in ffmpeg has its own arbitrary limitation on the number
+ * of strips, currently 32
+ */
 
 typedef enum {
 MODE_V1_ONLY = 0,
@@ -114,12 +116,12 @@ typedef enum {
 } mb_encoding;
 
 typedef struct {
-int v1_vector;  //index into v1 codebook
-int v1_error;   //error when using V1 encoding
-int v4_vector[4];   //indices into v4 codebook
-int v4_error;   //error when using V4 encoding
-int skip_error; //error when block is skipped (aka copied 
from last frame)
-mb_encoding best_encoding;  //last result from calculate_mode_score()
+int v1_vector; /* index into v1 codebook */
+int v1_error;  /* error when using V1 encoding */
+int v4_vector[4];  /* indices into v4 codebook */
+int v4_error;  /* error when using V4 encoding */
+int skip_error;/* error when block is skipped (aka copied 
from last frame) */
+mb_encoding best_encoding; /* last result from calculate_mode_score() 
*/
 } mb_info;
 
 typedef struct {
@@ -146,15 +148,15 @@ typedef struct {
 uint64_t lambda;
 int *codebook_input;
 int *codebook_closest;
-mb_info *mb;//MB RD state
-int min_strips;  //the current limit
-int max_strips;  //the current limit
+mb_info *mb;/* MB RD state */
+int min_strips;  /* the current limit */
+int max_strips;  /* the current limit */
 #ifdef CINEPAKENC_DEBUG
-mb_info *best_mb;   //TODO: remove. only used for 
printing stats
+mb_info *best_mb;   /* TODO: remove. only used for 
printing stats */
 int num_v1_mode, num_v4_mode, num_mc_mode;
 int num_v1_encs, num_v4_encs, num_skips;
 #endif
-// options
+/* options */
 int max_extra_cb_iterations;
 int skip_empty_cb;
 int min_min_strips;
@@ -219,10 +221,12 @@ static av_cold int cinepak_encode_init(AVCodecContext 
*avctx)
 
 mb_count = avctx->width * avctx->height / MB_AREA;
 
-//the largest possible chunk is 0x31 with all MBs encoded in V4 mode
-//and full codebooks being replaced in INTER mode,
-// which is 34 bits per MB
-//and 2*256 extra flag bits per strip
+/*
+ * the largest possible chunk is 0x31 with all MBs encoded in V4 mode
+ * and full codebooks being replaced in INTER mode,
+ * which 

[FFmpeg-devel] [PATCH 1/3] libavcodec/cinepakenc.c: comments cleanup (contents)

[u-9iep]

Attaching the new version.

Regards,
Rune
>From f563e57f7609cd890b655cb9d140849f0917a6d3 Mon Sep 17 00:00:00 2001
From: Rl 
Date: Sun, 29 Jan 2017 16:40:27 +0100
Subject: [PATCH 1/3] libavcodec/cinepakenc.c: comments cleanup (contents)

Change the encoding of the original developer name from ISO-8859-1 to UTF-8.
Remove the stale/completed TODO list.
Fix two small typos.
---
 libavcodec/cinepakenc.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/libavcodec/cinepakenc.c b/libavcodec/cinepakenc.c
index 3670157dfc..a28f669070 100644
--- a/libavcodec/cinepakenc.c
+++ b/libavcodec/cinepakenc.c
@@ -1,5 +1,5 @@
 /*
- * Cinepak encoder (c) 2011 Tomas H�rdin
+ * Cinepak encoder (c) 2011 Tomas Härdin
  * http://titan.codemill.se/~tomhar/cinepakenc.patch
  *
  * Fixes and improvements, vintage decoders compatibility
@@ -23,9 +23,6 @@ OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR 
OTHERWISE,
 ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
 OTHER DEALINGS IN THE SOFTWARE.
 
- * TODO:
- * - optimize: color space conversion, ...
- * - implement options to set the min/max number of strips?
  * MAYBE:
  * - "optimally" split the frame into several non-regular areas
  *   using a separate codebook pair for each area and approximating
@@ -92,7 +89,7 @@ OTHER DEALINGS IN THE SOFTWARE.
 #define MAX_STRIPS  32  //Note: having fewer choices regarding the number 
of strips speeds up encoding (obviously)
 #define MIN_STRIPS  1   //Note: having more strips speeds up encoding the 
frame (this is less obvious)
 // MAX_STRIPS limits the maximum quality you can reach
-//when you want hight quality on high resolutions,
+//when you want high quality on high resolutions,
 // MIN_STRIPS limits the minimum efficiently encodable bit rate
 //on low resolutions
 // the numbers are only used for brute force optimization for the first frame,
@@ -119,7 +116,7 @@ typedef enum {
 typedef struct {
 int v1_vector;  //index into v1 codebook
 int v1_error;   //error when using V1 encoding
-int v4_vector[4];   //indices into v4 codebooks
+int v4_vector[4];   //indices into v4 codebook
 int v4_error;   //error when using V4 encoding
 int skip_error; //error when block is skipped (aka copied 
from last frame)
 mb_encoding best_encoding;  //last result from calculate_mode_score()
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] patch 1: comments cleanup in libavcodec/cinepakenc.c

[u-9iep]

On Sat, Jan 28, 2017 at 07:53:06PM +0100, Michael Niedermayer wrote:
> On Sat, Jan 28, 2017 at 11:42:24AM +0100, u-9...@aetey.se wrote:
> > Attaching the patch.
> 
> please provide a git compatible patch
> git format-patch / send-email
> 
> git is quite flexible and often takes diffs in mails too
> but unfortunately my git does not accept this one

Sigh. Thanks for trying!

Will pass the patches through git and come back.

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] patch 2: comments style cleanup in libavcodec/cinepakenc.c

[u-9iep]

On Sat, Jan 28, 2017 at 08:30:34PM +0100, Moritz Barsnick wrote:
> On Sat, Jan 28, 2017 at 11:50:27 +0100, u-9...@aetey.se wrote:
> > -// MAX_STRIPS limits the maximum quality you can reach
> > -//when you want high quality on high resolutions,
> > -// MIN_STRIPS limits the minimum efficiently encodable bit rate
> > -//on low resolutions
> > -// the numbers are only used for brute force optimization for the first 
> > frame,
> > -// for the following frames they are adaptively readjusted
> > -// NOTE the decoder in ffmpeg has its own arbitrary limitation on the 
> > number
> > -// of strips, currently 32
> > +/* MAX_STRIPS limits the maximum quality you can reach */
> > +/*when you want high quality on high resolutions, */
> > +/* MIN_STRIPS limits the minimum efficiently encodable bit rate */
> > +/*on low resolutions */
> > +/* the numbers are only used for brute force optimization for the first 
> > frame, */
> > +/* for the following frames they are adaptively readjusted */
> > +/* NOTE the decoder in ffmpeg has its own arbitrary limitation on the 
> > number */
> > +/* of strips, currently 32 */
> 
> If this is supposed to be cosmetic, it's extremely ugly. Does any other

You have a point. At this place the layout should be changed beyond
simply replacing the delimiters.

> ffmpeg code use this "style"?

:) No, this was not an intended style, just a reflection of the old one.

> I think you'd rather use:
> 
>   /*
>* Extremely long
>* multiline comment
>* using non-C++ style
>* comment delimiters.
>*/

Yes this is what should have been done, like it looks in other places
in the same file.

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] patch 3 (of 3): comment FIX in libavcodec/cinepak.c

[u-9iep]

Fix a plainly wrong (inverted) misleading comment in the cinepak decoder.

No code changes.

I kindly ask to apply this fix, which of course was due from the beginning.

Attaching the patch.

Regards,
Rune
--- libavcodec/cinepak.c.orig   2017-01-28 17:00:37.0 +0100
+++ libavcodec/cinepak.c2017-01-28 17:04:20.513123515 +0100
@@ -155,8 +155,8 @@
 }
 }
 }
-/* to get the correct picture for not-multiple-of-4 cases let us fill
- * each block from the bottom up, thus possibly overwriting the top line
+/* to get the correct picture for not-multiple-of-4 cases let us fill each
+ * block from the bottom up, thus possibly overwriting the bottommost line
  * more than once but ending with the correct data in place
  * (instead of in-loop checking) */
 
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] patch 2: comments style cleanup in libavcodec/cinepakenc.c

[u-9iep]

On Sat, Jan 28, 2017 at 12:31:33PM +0100, wm4 wrote:
> On Sat, 28 Jan 2017 11:50:27 +0100
> u-9...@aetey.se wrote:
> 
> > Comments style cleanup:
> >  - make all comments follow the same style (C-style)
> > 
> > No code changes, only improved consistency and clarity in the comments.
> > No changes in the comments besides whitespace and the syntactic delimiters.
> > 
> > The original file uses a mixture of C and C++ style comments, not for
> > clarity but for historical reasons (among others commenting in a hurry).
> > 
> > With the change applied the structure of the file is more consequent
> > and also makes easier a possible code reuse with different C compilers.
> > 
> > Attaching the patch.
> > 
> > Regards,
> > Rune
> 
> "//" comments are ok in the ffmpeg codebase, because it's C99.

Yes I knew this. The change is purely cosmetic and not overly important
but in my humble opinion useful.

A consequent style makes it easier to read and understand the code.

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] patch 2: comments style cleanup in libavcodec/cinepakenc.c

[u-9iep]

Comments style cleanup:
 - make all comments follow the same style (C-style)

No code changes, only improved consistency and clarity in the comments.
No changes in the comments besides whitespace and the syntactic delimiters.

The original file uses a mixture of C and C++ style comments, not for
clarity but for historical reasons (among others commenting in a hurry).

With the change applied the structure of the file is more consequent
and also makes easier a possible code reuse with different C compilers.

Attaching the patch.

Regards,
Rune
--- libavcodec/cinepakenc.c.orig2017-01-28 10:10:47.078999401 +0100
+++ libavcodec/cinepakenc.c 2017-01-28 10:28:24.433190895 +0100
@@ -80,22 +80,22 @@
 #define STRIP_HEADER_SIZE 12
 #define CHUNK_HEADER_SIZE 4
 
-#define MB_SIZE 4   //4x4 MBs
+#define MB_SIZE 4   /* 4x4 MBs */
 #define MB_AREA (MB_SIZE*MB_SIZE)
 
-#define VECTOR_MAX 6//six or four entries per vector depending on 
format
-#define CODEBOOK_MAX 256//size of a codebook
+#define VECTOR_MAX 6/* six or four entries per vector depending on 
format */
+#define CODEBOOK_MAX 256/* size of a codebook */
 
-#define MAX_STRIPS  32  //Note: having fewer choices regarding the number 
of strips speeds up encoding (obviously)
-#define MIN_STRIPS  1   //Note: having more strips speeds up encoding the 
frame (this is less obvious)
-// MAX_STRIPS limits the maximum quality you can reach
-//when you want high quality on high resolutions,
-// MIN_STRIPS limits the minimum efficiently encodable bit rate
-//on low resolutions
-// the numbers are only used for brute force optimization for the first frame,
-// for the following frames they are adaptively readjusted
-// NOTE the decoder in ffmpeg has its own arbitrary limitation on the number
-// of strips, currently 32
+#define MAX_STRIPS  32  /* Note: having fewer choices regarding the number 
of strips speeds up encoding (obviously) */
+#define MIN_STRIPS  1   /* Note: having more strips speeds up encoding the 
frame (this is less obvious) */
+/* MAX_STRIPS limits the maximum quality you can reach */
+/*when you want high quality on high resolutions, */
+/* MIN_STRIPS limits the minimum efficiently encodable bit rate */
+/*on low resolutions */
+/* the numbers are only used for brute force optimization for the first frame, 
*/
+/* for the following frames they are adaptively readjusted */
+/* NOTE the decoder in ffmpeg has its own arbitrary limitation on the number */
+/* of strips, currently 32 */
 
 typedef enum {
 MODE_V1_ONLY = 0,
@@ -114,12 +114,12 @@
 } mb_encoding;
 
 typedef struct {
-int v1_vector;  //index into v1 codebook
-int v1_error;   //error when using V1 encoding
-int v4_vector[4];   //indices into v4 codebooks
-int v4_error;   //error when using V4 encoding
-int skip_error; //error when block is skipped (aka copied 
from last frame)
-mb_encoding best_encoding;  //last result from calculate_mode_score()
+int v1_vector;  /* index into v1 codebook */
+int v1_error;   /* error when using V1 encoding */
+int v4_vector[4];   /* indices into v4 codebooks */
+int v4_error;   /* error when using V4 encoding */
+int skip_error; /* error when block is skipped (aka copied 
from last frame) */
+mb_encoding best_encoding;  /* last result from calculate_mode_score() 
*/
 } mb_info;
 
 typedef struct {
@@ -146,15 +146,15 @@
 uint64_t lambda;
 int *codebook_input;
 int *codebook_closest;
-mb_info *mb;//MB RD state
-int min_strips;  //the current limit
-int max_strips;  //the current limit
+mb_info *mb;/* MB RD state */
+int min_strips;  /* the current limit */
+int max_strips;  /* the current limit */
 #ifdef CINEPAKENC_DEBUG
-mb_info *best_mb;   //TODO: remove. only used for 
printing stats
+mb_info *best_mb;   /* TODO: remove. only used for 
printing stats */
 int num_v1_mode, num_v4_mode, num_mc_mode;
 int num_v1_encs, num_v4_encs, num_skips;
 #endif
-// options
+/* options */
 int max_extra_cb_iterations;
 int skip_empty_cb;
 int min_min_strips;
@@ -219,10 +219,10 @@
 
 mb_count = avctx->width * avctx->height / MB_AREA;
 
-//the largest possible chunk is 0x31 with all MBs encoded in V4 mode
-//and full codebooks being replaced in INTER mode,
-// which is 34 bits per MB
-//and 2*256 extra flag bits per strip
+/* the largest possible chunk is 0x31 with all MBs encoded in V4 mode */
+/* and full codebooks being replaced in INTER mode, */
+/* which is 34 bits per MB */
+/* and 2*256 extra flag bits per strip */
 

[FFmpeg-devel] patch 1: comments cleanup in libavcodec/cinepakenc.c

[u-9iep]

Comments cleanup:
 - change the encoding of the original developer name from ISO-8859-1 to UTF-8
 - remove the stale/completed TODO list
 - fix a typo

No code changes, only improved consistency in the comments.

I kindly ask to apply this cleanup, which of course was due from the beginning.

Attaching the patch.

Regards,
Rune
--- libavcodec/cinepakenc.c.orig2016-12-05 23:07:54.0 +0100
+++ libavcodec/cinepakenc.c 2017-01-28 10:10:47.078999401 +0100
@@ -1,5 +1,5 @@
 /*
- * Cinepak encoder (c) 2011 Tomas H�rdin
+ * Cinepak encoder (c) 2011 Tomas Härdin
  * http://titan.codemill.se/~tomhar/cinepakenc.patch
  *
  * Fixes and improvements, vintage decoders compatibility
@@ -23,9 +23,6 @@
 ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
 OTHER DEALINGS IN THE SOFTWARE.
 
- * TODO:
- * - optimize: color space conversion, ...
- * - implement options to set the min/max number of strips?
  * MAYBE:
  * - "optimally" split the frame into several non-regular areas
  *   using a separate codebook pair for each area and approximating
@@ -92,7 +89,7 @@
 #define MAX_STRIPS  32  //Note: having fewer choices regarding the number 
of strips speeds up encoding (obviously)
 #define MIN_STRIPS  1   //Note: having more strips speeds up encoding the 
frame (this is less obvious)
 // MAX_STRIPS limits the maximum quality you can reach
-//when you want hight quality on high resolutions,
+//when you want high quality on high resolutions,
 // MIN_STRIPS limits the minimum efficiently encodable bit rate
 //on low resolutions
 // the numbers are only used for brute force optimization for the first frame,
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] patch: the fastest video decoder ever? :)

[u-9iep]

Here comes a slightly better version, with rounding to nearest instead
of rounding down at color bits truncation. I was unable to reliably measure
any speed difference compared to the simplistic former version.

The output quality now fully corresponds to the capabilities of rgb565
and the decoding is 12 - 17% faster than to rgb24.

The most remarkable gain of course is by skipping any extra format
conversion when on an rgb565 screen.

Posting the patch for perusal of possible interested parties.

A runtime output format option might be worth to be added,
and possibly support for more output formats as well?

Cheers,
Rune

On Fri, Jan 06, 2017 at 02:13:41PM +0100, u-9...@aetey.se wrote:
> Hello,
> 
> On slow hardware a 16-bit framebuffer depth makes a remarkable difference
> in performance and still provides a good look.
> 
> When videos are to be played on slow terminals there is a single
> best speed performer, cinepak (tell me if you know a better codec in this
> respect, the only comparable one I know of is VQA-15, but there is no
> reasonable encoder for it, nor a safe decoder).
> 
> Unfortunately cinepak as present in ffmpeg yields rgb24 which has to be
> repacked depending on the framebuffer format.
> 
> The attached patch makes cinepak to provide rgb565 in native endianness.
> This cuts in half (!) the mplayer CPU consumption on i686 with Xorg
> in the corresponding mode/depth, compared to decoding to rgb24.
> 
> The optimization is possible because cinepak is a VQ codec and pixel
> format packing at codebook fill is much more efficient than at a later
> stage. (Thanks Michael for the suggestion, long ago).
> 
> Of course the picture quality is slightly affected if this decoder version
> is used with larger framebuffer depths. A run-time switch would be to
> prefer, if this optimization against all odds would be welcome upstream :)
> 
> I am posting this as a proof of concept, lacking run-time format
> switching. As said, this patch yields half CPU consumption or double
> speed at decoding for rgb565 terminals.
--- libavcodec/cinepak.c.rgb24  2017-01-27 11:54:31.316799141 +0100
+++ libavcodec/cinepak.c2017-01-27 12:25:33.674095602 +0100
@@ -31,6 +31,8 @@
  *
  * Cinepak colorspace support (c) 2013 Rl, Aetey Global Technologies AB
  * @author Cinepak colorspace, Rl, Aetey Global Technologies AB
+ * Cinepak rgb565 format (c) 2017 Rl, Aetey Global Technologies AB
+ * @author Cinepak rgb565, Rl, Aetey Global Technologies AB
  */
 
 #include 
@@ -42,8 +44,12 @@
 #include "avcodec.h"
 #include "internal.h"
 
+/* feel free to replace with a better mapping implementation
+ * (keeping in mind slow, not very "intelligent" hardware)
+ */
+#define PACK_RGB_RGB565(r,g,b) 
(((av_clip_uint8((r)+4)>>3)<<11)|((av_clip_uint8((g)+2)>>2)<<5)|(av_clip_uint8((b)+4)>>3))
 
-typedef uint8_t cvid_codebook[12];
+typedef uint16_t cvid_codebook[4]; /* in the native endian rgb565 format */
 
 #define MAX_STRIPS  32
 
@@ -73,13 +79,13 @@
 uint32_t pal[256];
 } CinepakContext;
 
-static void cinepak_decode_codebook (cvid_codebook *codebook,
+static void cinepak_decode_codebook (cvid_codebook *codebook, int 
palette_video,
  int chunk_id, int size, const uint8_t 
*data)
 {
 const uint8_t *eod = (data + size);
 uint32_t flag, mask;
 int  i, n;
-uint8_t *p;
+uint16_t *p;
 
 /* check if this chunk contains 4- or 6-element vectors */
 n= (chunk_id & 0x04) ? 4 : 6;
@@ -98,33 +104,36 @@
 }
 
 if (!(chunk_id & 0x01) || (flag & mask)) {
-int k, kk;
+int k;
 
 if ((data + n) > eod)
 break;
 
-for (k = 0; k < 4; ++k) {
-int r = *data++;
-for (kk = 0; kk < 3; ++kk)
-*p++ = r;
-}
-if (n == 6) {
-int r, g, b, u, v;
+if (n == 4)
+/* 8-bit palette indexes or otherwise grayscale values,
+ * they need different handling */
+if (palette_video)
+for (k = 0; k < 4; ++k)
+*p++ = *data++;
+else
+for (k = 0; k < 4; ++k) {
+int r = *data++;
+*p++ = PACK_RGB_RGB565(r,r,r);
+}
+else {
+int y[4], u, v;
+for (k = 0; k < 4; ++k)
+/* 8-bit luminance values */
+y[k] = *data++;
 u = *(int8_t *)data++;
 v = *(int8_t *)data++;
-p -= 12;
-for(k=0; k<4; ++k) {
-r = *p++ + v*2;
-g = *p++ - (u/2) - v;
-b = *p   + u*2;
-p -= 2;
-*p++ = av_clip_uint8(r);
-*p++ = av_clip_uint8(g);
-*p++ = av_clip_uint8(b);
-}
+for(k=0; k<4; ++k)
+  

[FFmpeg-devel] patch: the fastest video decoder ever? :)

[u-9iep]

Hello,

On slow hardware a 16-bit framebuffer depth makes a remarkable difference
in performance and still provides a good look.

When videos are to be played on slow terminals there is a single
best speed performer, cinepak (tell me if you know a better codec in this
respect, the only comparable one I know of is VQA-15, but there is no
reasonable encoder for it, nor a safe decoder).

Unfortunately cinepak as present in ffmpeg yields rgb24 which has to be
repacked depending on the framebuffer format.

The attached patch makes cinepak to provide rgb565 in native endianness.
This cuts in half (!) the mplayer CPU consumption on i686 with Xorg
in the corresponding mode/depth, compared to decoding to rgb24.

The optimization is possible because cinepak is a VC codec and pixel
format packing at codebook fill is much more efficient than at a later
stage. (Thanks Michael for the suggestion, long ago).

Of course the picture quality is slightly affected if this decoder version
is used with larger framebuffer depths. A run-time switch would be to
prefer, if this optimization against all odds would be welcome upstream :)

I am posting this as a proof of concept, lacking run-time format
switching. As said, this patch yields half CPU consumption or double
speed at decoding for rgb565 terminals.

Regards,
Rune
--- libavcodec/cinepak.c.rgb24  2017-01-05 14:43:53.379965430 +0100
+++ libavcodec/cinepak.c2017-01-05 21:54:04.333090225 +0100
@@ -30,7 +30,9 @@
  *   http://wiki.multimedia.cx/index.php?title=Sega_FILM
  *
  * Cinepak colorspace support (c) 2013 Rl, Aetey Global Technologies AB
+ * Cinepak rgb565 format (c) 2017 Rl, Aetey Global Technologies AB
  * @author Cinepak colorspace, Rl, Aetey Global Technologies AB
+ * @author Cinepak rgb565, Rl, Aetey Global Technologies AB
  */
 
 #include 
@@ -42,8 +44,12 @@
 #include "avcodec.h"
 #include "internal.h"
 
+/* feel free to replace with a better mapping implementation
+ * (keeping in mind slow, not very "intelligent" hardware)
+ */
+#define PACK_RGB_RGB565(r,g,b) r)>>3)<<11)|(((g)>>2)<<5)|(b>>3))
 
-typedef uint8_t cvid_codebook[12];
+typedef uint16_t cvid_codebook[4]; /* in the native endian rgb565 format */
 
 #define MAX_STRIPS  32
 
@@ -73,13 +79,13 @@
 uint32_t pal[256];
 } CinepakContext;
 
-static void cinepak_decode_codebook (cvid_codebook *codebook,
+static void cinepak_decode_codebook (cvid_codebook *codebook, int 
palette_video,
  int chunk_id, int size, const uint8_t 
*data)
 {
 const uint8_t *eod = (data + size);
 uint32_t flag, mask;
 int  i, n;
-uint8_t *p;
+uint16_t *p;
 
 /* check if this chunk contains 4- or 6-element vectors */
 n= (chunk_id & 0x04) ? 4 : 6;
@@ -98,33 +104,36 @@
 }
 
 if (!(chunk_id & 0x01) || (flag & mask)) {
-int k, kk;
+int k;
 
 if ((data + n) > eod)
 break;
 
-for (k = 0; k < 4; ++k) {
-int r = *data++;
-for (kk = 0; kk < 3; ++kk)
-*p++ = r;
-}
-if (n == 6) {
-int r, g, b, u, v;
+if (n == 4)
+/* 8-bit palette indexes or otherwise grayscale values,
+ * they need different handling */
+if (palette_video)
+for (k = 0; k < 4; ++k)
+*p++ = *data++;
+else
+for (k = 0; k < 4; ++k) {
+int r = *data++;
+*p++ = PACK_RGB_RGB565(r,r,r);
+}
+else {
+int y[4], u, v;
+for (k = 0; k < 4; ++k)
+/* 8-bit luminance values */
+y[k] = *data++;
 u = *(int8_t *)data++;
 v = *(int8_t *)data++;
-p -= 12;
-for(k=0; k<4; ++k) {
-r = *p++ + v*2;
-g = *p++ - (u/2) - v;
-b = *p   + u*2;
-p -= 2;
-*p++ = av_clip_uint8(r);
-*p++ = av_clip_uint8(g);
-*p++ = av_clip_uint8(b);
-}
+for(k=0; k<4; ++k)
+*p++ = PACK_RGB_RGB565(av_clip_uint8(y[k] + v*2),
+   av_clip_uint8(y[k] - (u/2) - v),
+   av_clip_uint8(y[k] + u*2));
 }
 } else {
-p += 12;
+p += 4;
 }
 }
 }
@@ -134,8 +143,8 @@
 {
 const uint8_t   *eod = (data + size);
 uint32_t flag, mask;
-uint8_t *cb0, *cb1, *cb2, *cb3;
-int x, y;
+uint16_t*cb0, *cb1, *cb2, *cb3;
+int  x, y;
 char*ip0, *ip1, *ip2, *ip3;
 
 flag = 0;
@@ -145,7 +154,7 @@
 
 /* take care of y dimension not being multiple of 4, such streams exist */
 ip0 = ip1 = ip2 = 

Re: [FFmpeg-devel] [PATCH 09/13] avcodec/svq1dec: clear MMX state after MB decode loop

[u-9iep]

On Wed, Oct 26, 2016 at 06:17:27PM +0200, wm4 wrote:
> > With all respect, I find such an argument hardly appropriate for two 
> > reasons:
> > it is formally incorrect (see below),
> > it shifts the attention from the functionality/usability to emotionally
> > charged aspects like age.
> 
> It's not an "emotionally charged" aspect. Phasing out old hardware is
> common sense. Of course this doesn't appeal to the "keep all the trash"

You do not seem to notice that you make the same mistake,
using emotionally charged expressions.

You are insulting the people who use older hardware, which is
_not_necessarily_ "trash" and who do this for their reasons - good or
bad is not in the realm of one's competence unless one is involved in
the actual case.

You the developers know everything between CPU, memory and I/O. The user
acts in her real world. For the developers it is unfounded to pretend
to be familiar with all the worlds out there.

If not otherwise, I mentioned the good reasons to run on the existing
hardware:

On Wed, Oct 26, 2016 at 06:04:24PM +0200, u-9...@aetey.se wrote:
> Such arguments can not be passed on to the users without an offer of
> a corresponding amount in cache. :)
> 
> More seriously, the act of replacement itself has its costs too, in
> cases like this one such cost is much higher than the cost of hardware.

To make it clear why I do not offer the expected spared money to the
developers, let me point out the phrase which may have gone unnoticed:

> (regrettably, this cost or gain is not trivial to convert to a
> compensation for the ffmpeg developers, unless video is one's main
> business, sorry for that!)
=>^^

I did my best to contribute some small bits of code and of understanding.

To conclude, I would like to friendly suggest to every bright
programmer on this list (sincerely highly respecting your skills),
try to notice when you tread outside your competence area.

Good luck to the ffmpeg project, my humble effort here is over.

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 09/13] avcodec/svq1dec: clear MMX state after MB decode loop

[u-9iep]

On Wed, Oct 26, 2016 at 05:08:56PM +0200, Hendrik Leppkes wrote:
> > One of the highly appreciated virtues of ffmpeg is its efficiency,
> > this makes hardware much more useful. Please do not cut off the low
> > end systems when possible.

> Its not about low-end, its about 15+ years old hardware. At some point

With all respect, I find such an argument hardly appropriate for two reasons:
it is formally incorrect (see below),
it shifts the attention from the functionality/usability to emotionally
charged aspects like age.

I am participating in the discussion as a concerned _packager_.
Nevertheless I am a ffmpeg "user" too.

As a matter of fact I am running several low power units which have mmx but no 
sse.
Several of them exist for handling video, either for playback or encoding or 
both.

They have been set up this way because they fit the bill very nicely,
among others thanks to ffmpeg.

The one I looked at recently is manufactured 10 years ago and is not
youngest in its generation. So we have to wait at least extra 5 years
to make it 15+.

> you just have to replace this stuff.
> For $100 you can buy a system at least 10 times as fast then those.
> Heck even a RPi might rival those for $20 or so.

Such arguments can not be passed on to the users without an offer of
a corresponding amount in cache. :)

More seriously, the act of replacement itself has its costs too, in
cases like this one such cost is much higher than the cost of hardware.

(regrettably, this cost or gain is not trivial to convert to a
compensation for the ffmpeg developers, unless video is one's main
business, sorry for that!)

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 09/13] avcodec/svq1dec: clear MMX state after MB decode loop

[u-9iep]

On Wed, Oct 26, 2016 at 04:21:14PM +0200, Hendrik Leppkes wrote:
> You can add "not caring about first-gen sse2 CPUs" to the list as
> well, if you want. Those are way old as well.
> There is going to be a performance loss either way, except that emms
> slows it down everywhere, while using sse2 is likely to be fine on
> modern CPUs. So IMHO thats the better course to take, if any.

Note that making MMX-acceleration unavailable would hurt at most on
those slowest, non-sse systems, which otherwise e.g. often struggle for
real-time decoding of heavier streams.

One of the highly appreciated virtues of ffmpeg is its efficiency,
this makes hardware much more useful. Please do not cut off the low
end systems when possible.

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 09/13] avcodec/svq1dec: clear MMX state after MB decode loop

[u-9iep]

On Tue, Oct 25, 2016 at 05:48:50PM +0200, Henrik Gramner wrote:
> On Tue, Oct 25, 2016 at 2:28 PM,   wrote:
> > It would be nice to look at a benchmarking comparison, to be able to
> > see the actual practical performance gain of the decision not to follow
> > the ABI.
> 
> Just a quick comparison from adding EMMS to a random MMX function
> (from x264, because I happened to have the source for that but not
> ffmpeg on this machine and I was too lazy to download it).
> 
> sad_4x4_c: 359
> sad_4x4_mmx: 94
> sad_4x4_mmx (emms): 186

This is unfortunatly not exactly the numbers a user or a packager would
need to know. The relevant ones would be what time it takes to actually
compress or decompress a given file.

(if most of the time is spent calling this very function, then we can
extrapolate the numbers to the net result, otherwise we shouldn't)

If we nevertheless assume that the net performance is proportional
to the numbers above, we learn that omission of MMX acceleration
compatible with the standard ABI would lead to 50% performance loss
for the ABI-compliant users (pure C instead of compliant MMX).

This would be a pity, even if the circle of the users who depend
on strict ABI-compliance is limited, for the moment.

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 09/13] avcodec/svq1dec: clear MMX state after MB decode loop

[u-9iep]

On Mon, Oct 24, 2016 at 09:53:22PM +0200, Henrik Gramner wrote:
> The decision to issue emms manually instead of after every MMX
> function was a deliberate decision. I'd hardly call it "misdesigned"
> to make SIMD code twice as fast at the cost of technically abusing the

It would be nice to look at a benchmarking comparison, to be able to
see the actual practical performance gain of the decision not to follow
the ABI.

My expectation is among others that using MMX in a compliant way would
remain remarkably beneficial compared to no SIMD.

In a perfect world the user might be offered a build time choice:
either lose N% of the performance or take the consequences of not
following the ABI. Apparently, different users/packagers would make
different choices, but the crucial thing would be knowing the N.

Of course, offering to disable MMX _is_ such a choice, but having instead
an option of "somewhat slower, compliant MMX" would be much nicer.

my 2c
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] ogg kate text subtitles support (patch)

[u-9iep]

On Mon, Oct 24, 2016 at 11:49:25AM +0200, Clément Bœsch wrote:
> > It is bad if you don't strip the markup in the decoder.

> To expand on this: it is extremely worrisome that ffmpeg could be the
> source of yet another wave of insane .srt files with kate markup in it
> because someone decided to run ffmpeg -i in.ogg out.srt in order to
> extract the subtitles and share them.
> 
> You absolutely want to strip all markup in the decoder for minimal
> support, or FFmpeg will generate broken files, which is not tolerable.

I see and have to agree.

(even though I would not expect any remarkable harm to happen in practice)

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] ogg kate text subtitles support (patch)

[u-9iep]

On Mon, Oct 24, 2016 at 11:46:07AM +0200, Clément Bœsch wrote:
> On Mon, Oct 24, 2016 at 11:39:40AM +0200, u-9...@aetey.se wrote:
> > Yes, you are right, the patch just ignores the possible presence
> > of Kate markup (does not try to interpret it, nor removes).
> > This is probably not too bad, for the minimal support.
> 
> It is bad if you don't strip the markup in the decoder.

It shouldn't be hard (the kate_text_remove_markup() function in libkate
is just 49 lines) but was not included in the original patch.

Could be added later if desired, either as an incremental patch or by
introducing an optional dependency on libkate (which would open for full
Kate functionality).

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] ogg kate text subtitles support (patch)

[u-9iep]

On Mon, Oct 24, 2016 at 11:09:43AM +0200, wm4 wrote:
> > Text subtitles in ogg kate are a straightforward way to put srt-like data
> 
> Note that srt supports simple HTML-like tags.

Yes, you are right, the patch just ignores the possible presence
of Kate markup (does not try to interpret it, nor removes).
This is probably not too bad, for the minimal support.

Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] ogg kate text subtitles support (patch)

[u-9iep]

worth to mention:

On Mon, Oct 24, 2016 at 10:25:44AM +0200, u-9...@aetey.se wrote:
> > >https://trac.ffmpeg.org/ticket/3039

The ticket refers to a sample with graphical subtitles. This is _not_
being solved by the proposed patch.

A sample with text subtitles can be seen e.g. at
 http://people.xiph.org/~oggk/elephants_dream/elephantsdream-with-subtitles.ogg
It is well decodable with the patch applied.

Text subtitles in ogg kate are a straightforward way to put srt-like data
into ogg. Multiplexing kate into ogg is not addressed by the proposed
patch, but is otherwise easily done by external tools. Such tools are
not applicable as much during playback, that's why demuxing support
is most important.

Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] ogg kate text subtitles support (patch)

[u-9iep]

On Mon, Oct 24, 2016 at 10:00:38AM +0200, wm4 wrote:
> > The patch addresses the missing Kate subtitles support, reflected
> > in trac as
> >https://trac.ffmpeg.org/ticket/3039

> I don't quite get it. Doesn't this just demux kate subtitles as text
> without stripping whatever formatting tags kate might support? (I was
> under the impression that kate does more than just plain text.)

Kate can of course do much more than plain text, but this patch only
adds the recognition of ogg-kate-text, nothing else. That's what makes
it a low hanging fruit.

At the same time we get a remarkable improvement, compared to the complete
inability to handle subtitles in ogg.

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] lurking bugs in the mmx-related assembler code (?)

[u-9iep]

On Mon, Oct 03, 2016 at 09:48:47PM +0200, Nicolas George wrote:
> > As bright as the people here are, they land in a foreign area, which
> > accidentally leads to statements like the above.
> 
> I am sorry, but an appeal to authority will not cut it in front of real
> arguments.

[Everyone, sorry for offtopic]

It was not an appeal to authority but a warning, trying to reduce the
number of nice and intelligent people making fools of themselves.

> The real argument here is: musl makes several assumptions about the
> architecture and compiler (for example: that floats and ints have the same

You should have noticed that it is actually generated per architecture
and has requirements on compilers.

> endianness) that are not mandated by any standard. And although these

Hmm isn't there such a thing called the CPU specification?

> assumptions are very reasonable and widely respected, there will eventually
> be an architecture or, more probably, a compiler, that will break them.

You mean if you _choose_ to build it for a wrong architecture or by a
wrong compiler? Then yes. :) But not otherwise.

When built properly it will fully cooperate with applications which
conform to standards. Any of them.

Unfortunately, this apparently can not be said about ffmpeg.
Replacing one standard-conforming library with another one can
apparently lead to a crash.

> FFmpeg does exactly the same.

Alas, not really.

> In this instance, we know the reason for FFmpeg: resetting the FPU state is
> expensive, and this is speed-critical code. Please tell us what are musl's
> reasons for using this ugly hack. If the answer is speed, I would very much

Be careful before calling any code "ugly". May be you just do not
understand it.

And concerning why it looks this way - this is _irrelevant_ here.
Don't try to blame the error of the old vp3 code on musl.

> like to see a benchmark comparing this implementation to a portable but
> optimized log2, especially for x86_32, where  int <-> float conversions are
> very expensive.

You are welcome to learn why the things are done in a certain way.
A good place to start might be the musl mailing list.

Yours,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]doc/platform: Mention musl where x86_32 is not supported

[u-9iep]

On Mon, Oct 03, 2016 at 07:28:08PM +0200, Hendrik Leppkes wrote:
> > Again: no offence! Standard libraries are just a quite different area,
> > it postulated other skills and presents other implementation challenges
> > than multimedia programming.
> 
> Optimized code is the same everywhere, you just write different algorithms.

Note "the same everywhere".

Optimization goals, constraints and strategies are different in different
domains.

No one is an expert in all of the various domains. Not me, not you.

Yes it is hard to accept this, especially for very bright people who really
know exceptionally many things - but "a lot" is not enough for everywhere.

I hope we can stop discussing musl internals, there is no reason to do it
on this list, nor other prerequisits for the discussion to make sense.

As a concerned user of ffmpeg on 32-bit Intel I plead to make ffmpeg
fully usable on this platform regardless of the internals of the C
library being used.

IOW I plead to make ffmpeg conform to the standards. My expectation
and hope is that this can be done without making a noticeable performance
sacrifice.

Relying on C library internals is a hack, and not a robust one.

Yours,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]doc/platform: Mention musl where x86_32 is not supported

[u-9iep]

On Mon, Oct 03, 2016 at 05:27:19PM +0200, wm4 wrote:
> > Musl merely showed you the problem and now you are suggesting to "document
> > that the messenger with his bad news about our health is no longer welcome".
> 
> musl could also choose to abandon its incredibly "clever" hack (that
> makes almost everyone who sees it go "WTF"), and, as was pointed our on

:-D
IOW it exposes the _bugs_ in how people are accustomed to reason.

> IRC, probably increase the efficiency and readability of the code in
> question. Yes, musl is technically in the right, but only technically.

Is your opinion (about efficiency) sufficiently well informed?
No offence but I assume you to be a mutimedia guru, not a standard
library expert, which the author of the code is.

We shall not blindly accept authority of course. If you have a suggestion
for improvement, given a reasonable ground it would be useful on the musl
mailing list.

Here and without a real ground it looks like a FUD.

As for readability, it is your personal opinion.

Again: no offence! Standard libraries are just a quite different area,
it postulated other skills and presents other implementation challenges
than multimedia programming.

Friendly regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]doc/platform: Mention musl where x86_32 is not supported

[u-9iep]

Ronald,

I sincerely appreciate that you are taking the effort to talk to me
and explain your position.

Unfortunately in your messages you consistently ignored a crucial point
and this is the last time I try to retell it:

> On Mon, Oct 3, 2016 at 10:26 AM,  wrote:
> > What you are talking about is to ask Rich to modify musl to hide ffmpeg's
> > non-compliance bug (which glibc/uclibc do by sheer luck but this may change
> > any time).

Which you answer with

> - or we can just do what we do now and work with musl people to change
> their code.

Once again, musl has absolutely no reason to change its code,
their code is well done and fully compliant.

Their responsibility is rather to the contrary, _not_ to make changes
which would hide sneaky bugs in applications. UB is a quite hideous bug.

> If it turns out the first two are undoable and musl devs are
> unwilling to do this, then we'll have to reconsider Carl's patch and call
> musl on x86-32 unsupported, with a link to the relevant discussion to back

You can not just "call musl on x86-32 unsupported", the only honest option
is to document that ffmpeg is not compliant/safe.

The problem is not in musl.
Last time in this discussion let me try to make this fact understood:

Musl merely showed you the problem and now you are suggesting to "document
that the messenger with his bad news about our health is no longer welcome".

> Again, this requires some time and thought.

This sounds very reasonable.

Best regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]doc/platform: Mention musl where x86_32 is not supported

[u-9iep]

On Mon, Oct 03, 2016 at 04:26:50PM +0200, u-9...@aetey.se wrote:
> With all the competence present here, is it really infeasible to improve
> the code structure so that it does not involve the C library in the
> bit-crunching performance critical paths??

Bad wording. Should be: "assembler-implemented" paths.

There is nothing wrong with using C library's routines - they are well
optimized. But if they are relied upon, they have to be respected,
they assume namely that the caller behaves according to the contract.

If you have a real need to do things which are incompatible with the
API contract, then the C library simply is not available.

Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]doc/platform: Mention musl where x86_32 is not supported

[u-9iep]

On Mon, Oct 03, 2016 at 08:01:05AM -0400, Ronald S. Bultje wrote:
> > Ping on the patch:

> The patch is unlikely to assist in fixing the issue and is likely to cause
> further inflammation. Therefore I would prefer you did not apply the patch
> and also please don't send a new version that is differently worded.
> 
> I would prefer to work with upstream (musl) and fix the issue where ffmpeg
> running with musl crashes. Whether that means changing ffmpeg or musl
> remains to be seen. Let's chat with the developers and figure something out.

The standard C library API is what it is called, a standard.

What you are talking about is to ask Rich to modify musl to hide ffmpeg's
non-compliance bug (which glibc/uclibc do by sheer luck but this may change
any time).

With all the competence present here, is it really infeasible to improve
the code structure so that it does not involve the C library in the
bit-crunching performance critical paths??

Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]doc/platform: Mention musl where x86_32 is not supported

[u-9iep]

On Mon, Oct 03, 2016 at 12:37:29PM +0200, Carl Eugen Hoyos wrote:
> 2016-10-03 12:30 GMT+02:00 Hendrik Leppkes :
> 
> > Lets just fix the bugs
> 
> Sorry: Which bugs exactly should we just fix?
> 
> Carl Eugen

Please fix the UB in the MMX-code.

Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] lurking bugs in the mmx-related assembler code (?)

[u-9iep]

> > http://git.musl-libc.org/cgit/musl/tree/src/malloc/malloc.c#n114
> 
> Urgh. This is even worse than I imagined. FFmpeg is using undefined
> behaviours by calling it without resetting the state, but this is also
> completely undefined behaviour on their side.

I feel a duty to remind, in the most positive and friendly tone:

The author of the referred code acts in his actual area of competence
(C libraries and standards).

The comments here on the C library code and standard compliance come from
developers having a different competence area (multimedia programming).

As bright as the people here are, they land in a foreign area, which
accidentally leads to statements like the above.

Regards,
Rune

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel