[FFmpeg-devel] [PATCH 2/2] pixdesc: deprecate AV_PIX_FMT_FLAG_PSEUDOPAL

wm4 nfxjfg at googlemail.com
Fri Mar 30 16:10:17 EEST 2018


On Fri, 30 Mar 2018 13:22:25 +0200
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Fri, Mar 30, 2018 at 03:23:25AM +0200, wm4 wrote:
> > On Fri, 30 Mar 2018 03:13:07 +0200
> > Michael Niedermayer <michael at niedermayer.cc> wrote:
> >   
> > > On Thu, Mar 29, 2018 at 03:30:43PM +0200, wm4 wrote:  
> > > > PSEUDOPAL pixel formats are not paletted, but carried a palette with the
> > > > intention of allowing code to treat unpaletted formats as paletted. The
> > > > palette simply mapped the byte values to the resulting RGB values,
> > > > making it some sort of LUT for RGB conversion.
> > > > 
> > > > It was used for 1 byte formats only: RGB4_BYTE, BGR4_BYTE, RGB8, BGR8,
> > > > GRAY8. The first 4 are awfully obscure, used only by some ancient bitmap
> > > > formats. The last one, GRAY8, is more common, but its treatment is
> > > > grossly incorrect. It considers full range GRAY8 only, so GRAY8 coming
> > > > from typical Y video planes was not mapped to the correct RGB values.
> > > > Also, nothing actually used the PSEUDOPAL palette data, except xwdenc.
> > > > All other code had to treat it as a special case, just to ignore or
> > > > propagate palette data.
> > > > 
> > > > In conclusion, this was just a very strange old hack that has no real
> > > > justification to exist. It's negatively useful, because API users who
> > > > allocate their own pixel data have to be aware that they need to
> > > > allocate the palette, or FFmpeg will crash on them in _some_ situations.
> > > > On top of this, there was no API to allocate the pseuo palette outside
> > > > of av_frame_get_buffer(). (avpriv_set_systematic_pal2() was not public,
> > > > which is good, because GRAY8 treatment is broken.)
> > > > 
> > > > This patch not only deprecates AV_PIX_FMT_FLAG_PSEUDOPAL, but also makes
> > > > the pseudo palette optional. Nothing accesses it anymore, though if it's
> > > > set, it's propagated. It's still allocated and initialized for
> > > > compatibility with API users that rely on this feature. But new API
> > > > users do not need to allocate it. This was an explicit goal of this
> > > > patch.
> > > > 
> > > > Most changes replace AV_PIX_FMT_FLAG_PSEUDOPAL with FF_PSEUDOPAL. I
> > > > first tried #ifdefing all code, but it was a mess. The FF_PSEUDOPAL
> > > > macro reduces the mess, and still allows defining FF_API_PSEUDOPAL to 0.
> > > > 
> > > > Passes FATE with FF_API_PSEUDOPAL enabled and disabled. In addition,
> > > > FATE passes with FF_API_PSEUDOPAL set to 1, but with allocation
> > > > functions manually changed to not allocating a palette.
> > > > ---    
> > > 
> > > iam not sure if your rants / political views belong in a commit message.
> > > I think they should be removed.   
> > 
> > There are no "political" views. Please point out which parts you think
> > are political, and why they supposedly are political.
> > 
> > There are no rants either. In fact, calling them rants is disrespectful
> > and implies there is no logic behind whatever parts you think are rants.  
> 
> you made disrespectful comments in the commit message above.
> That is implying code others have written is
> "this was just a very strange old hack that has no real justification to exist"
> "It's negatively useful"

This is because you take this personally for some reason.

This thing might have had a justification in the old days, when these 1
byte formats made up a significant part of all pixfmts, and handling
some as simply paletted could possibly have simplified code.

You have to admit that carrying a static colorspace conversion LUT
(just to RGB) as a palette as additional AVFrame data pointer is a bit
strange, and could also be called a hack. Let me know if you disagree.

My point is that this mechanism hasn't well aged, and any
expectations the original author might have had from this weren't
fulfilled (or at least not anymore in recent times). Indeed, there was
no code in FFmpeg that even relied on this, except xwdenc (which in
itself has a questionable usefulness, because it's an encoder for the
mostly-raw format of a certain X11 screenshot utility that probably
nobody uses anymore - reading is already a stretch, but encoding is
likely not to be used by anyone).

In the newest time, API users are surprised by this pseudopal
mechanism, and ran into it only because _some_ FFmpeg code crashed in
AVFrame with pseudopal formats that didn't have a palette (source: it
happened to me). As such, it doesn't really help anyone (see above),
but actually causes more work to some (see this paragraph). So I think
"negatively useful" is a justified expression and is not disrespectful
to anyone. (Who would I even address? I don't even know who the
original author was. I just looked and it's actually hard to determine.
I only see a commit by a Libav developer in 2012, which added the flag
to complement the already existing mechanism. As far as disrespect
goes, there is/has been plenty of _actual_ disrespect against Libav
developers in the FFmpeg, and you never did anything against it when
you were still project leader.)

Calling my detailed commit message that intends to present the full
situation in a complete way a "rant" is really very insulting. Do you
think I'm some sort of clown who writes commit messages for fun and to
annoy people?

Why do you try to incite hostilities and discord?

It could have been a technical discussion, but you start things like
this.

> iam not sure if you are trying to pick a fight or what the point of these is
> but
> 
> These are clearly not technical comments nor are they correct.
> The code had sense, was usefull, and allowed other code to be simplified.

Yeah, I see past tense. Like I pointed out above, there was only xwdenc
which used this, and it was trivially replaced. All other code was only
propagating the palette or making space for allocation.

> I called them political rants, maybe they are something else but either way
> i do not belive this belongs in a commit message.

No, this definitely belongs into a commit message:
- What mechanism is changed?
- Why was the mechanism useful?
- Why is the mechanism not useful anymore?
- Why should it be changed at all?

I listed all these things. It's not a "rant". It's hilarious how you
always complain that I'm supposedly disrespectful to you, but then you
always do things like this.

> 
> 
> 
> >   
> > > about the patch, ive not tested it yet or looked deeper but
> > > please seperate the identifer renaming out into its own patch, that way
> > > it will be much more readable.  
> > 
> > There's nothing being renamed.   
> 
> Of course there is, the whole patch is full of changes like this:
> 
> @@ -423,7 +425,7 @@ static int raw_decode(AVCodecContext *avctx, void *data, int *got_frame,
>      }
> 
>      if ((avctx->pix_fmt == AV_PIX_FMT_PAL8 && buf_size < context->frame_size) ||
> -        (desc->flags & AV_PIX_FMT_FLAG_PSEUDOPAL)) {
> +        (desc->flags & FF_PSEUDOPAL)) {
>          frame->buf[1]  = av_buffer_ref(context->palette);
>          if (!frame->buf[1]) {
>              av_buffer_unref(&frame->buf[0]);
> 
> Here above its clearly vissible but in many hunks its intermixed with other
> changes making the patch hard to read

Have you read my commit message? The alternative is littering the code
with #ifdefs which is even less readable. This is due to the FFmpeg
policy that deprecated code needs to be conditionally compiled, based
on FF_API_ flags. I don't think it's appropriate to move this into a
separate patch, just to reduce the size of the patch.

Surely you will be able to read and understand it anyway?

Should we discuss how unreadable the merge commits are that you made for
years?

>              
> > This is the deprecation. Do you prefer if
> > I litter the code with ifdefs instead? Did you read the commit message?  
> 
> I did read the commit message, otherwise how could i complain about it?

By reading it selectively and ignoring other parts.

> 
> >   
> > > 
> > > thx  
> > 
> > If you meant it you'd do a proper review.  
> 
> Please stop with these disrespectful snide comments.

Same to you.

> I intended to review it once the renaming is split out.

I don't intend to make such strange changes. If you don't have anything
actual to contribute, I will push this patch on monday.


More information about the ffmpeg-devel mailing list