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

[Paul B Mahol]

On 3/30/18, 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"
>
> 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

Why you and some other 'old' developers have urge to block every single patch
that comes from some developers?