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

wm4 nfxjfg at googlemail.com
Fri Mar 30 04:23:25 EEST 2018


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.

> 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. This is the deprecation. Do you prefer if
I litter the code with ifdefs instead? Did you read the commit message?

> 
> thx

If you meant it you'd do a proper review.


More information about the ffmpeg-devel mailing list