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

[Michael Niedermayer]

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"

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.

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



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

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


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

Please stop with these disrespectful snide comments.
I intended to review it once the renaming is split out.

thank you

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are best at talking, realize last or never when they are wrong.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180330/03e3cc50/attachment.sig>