[FFmpeg-devel] [PATCH 2/4] avfilter/avfiltergraph: fix has_alpha in pick_format

[Paul B Mahol]

On 4/24/18, wm4 <nfxjfg at googlemail.com> wrote:
> On Tue, 24 Apr 2018 20:23:43 +0200 (CEST)
> Marton Balint <cus at passwd.hu> wrote:
>
>> On Tue, 24 Apr 2018, Michael Niedermayer wrote:
>>
>> > On Tue, Apr 24, 2018 at 02:10:53AM +0200, Michael Niedermayer wrote:
>> >> On Mon, Apr 23, 2018 at 04:50:54AM +0200, Marton Balint wrote:
>> >>>
>> >>>
>> >>> On Mon, 23 Apr 2018, Michael Niedermayer wrote:
>> >>>
>> >>>> On Sun, Apr 22, 2018 at 01:44:19PM +0200, Marton Balint wrote:
>> >>>>>
>> >>>>>
>> >>>>> On Fri, 20 Apr 2018, Michael Niedermayer wrote:
>> >>>>>
>> >>>>>> On Thu, Apr 19, 2018 at 09:32:19PM +0200, Marton Balint wrote:
>> >>>>>>> A pixel format which has a palette also has alpha, without this
>> >>>>>>> patch the
>> >>>>>>> format negotiation might have preferred formats without alpha even
>> >>>>>>> if the
>> >>>>>>> source had alpha.
>> >>>>>>>
>> >>>>>>> Signed-off-by: Marton Balint <cus at passwd.hu>
>> >>>>>>> ---
>> >>>>>>> libavfilter/avfiltergraph.c | 2 +-
>> >>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> >>>>>>>
>> >>>>>>> diff --git a/libavfilter/avfiltergraph.c
>> >>>>>>> b/libavfilter/avfiltergraph.c
>> >>>>>>> index 4cc6892404..e18f733e23 100644
>> >>>>>>> --- a/libavfilter/avfiltergraph.c
>> >>>>>>> +++ b/libavfilter/avfiltergraph.c
>> >>>>>>> @@ -679,7 +679,7 @@ static int pick_format(AVFilterLink *link,
>> >>>>>>> AVFilterLink *ref)
>> >>>>>>>
>> >>>>>>>    if (link->type == AVMEDIA_TYPE_VIDEO) {
>> >>>>>>>        if(ref && ref->type == AVMEDIA_TYPE_VIDEO){
>> >>>>>>> -            int has_alpha=
>> >>>>>>> av_pix_fmt_desc_get(ref->format)->nb_components % 2 == 0;
>> >>>>>>> +            int has_alpha=
>> >>>>>>> av_pix_fmt_desc_get(ref->format)->nb_components % 2 == 0 ||
>> >>>>>>> (av_pix_fmt_desc_get(ref->format)->flags & AV_PIX_FMT_FLAG_PAL);
>> >>>>>>
>> >>>>>> This causes various output files to grow in size with a unused
>> >>>>>> alpha plane
>> >>>>>>
>> >>>>>> a random example: (there are likels better examples)
>> >>>>>> ./ffmpeg -y -i ~/tickets/1116/1023.bmp -vf negate fileX.bmp
>> >>>>>>
>> >>>>>> Iam not sure unconditionally treating all palettes as if they have
>> >>>>>> non fully opaque entries is a good idea.
>> >>>>>
>> >>>>> Obviously not, but it is already treated this way in most places.
>> >>>>> Having a
>> >>>>> bigger image with alpha is better than losing alpha. And the user
>> >>>>> can always
>> >>>>> force losing alpha with a filter, and sometimes he has to do that
>> >>>>> anyway
>> >>>>> because for example fre0r filters have no way of signalling if they
>> >>>>> use
>> >>>>> alpha or not...
>> >>>>
>> >>>> you can, the average user certainly doesnt have the knowledge to
>> >>>> adjust
>> >>>> anything alpha by hand, the average user isnt even aware of that the
>> >>>> issue
>> >>>> is alpha or pal8 related probably
>> >>>>
>> >>>> also about "better", i saw a few cases that got bigger, i dont
>> >>>> remember
>> >>>> seeing a case that was fixed.
>> >>>> Have you seen real usecases this fixes ?
>> >>>
>> >>> A source file with a palette and alpha and a filter which supports
>> >>> formats
>> >>> with both alpha and without:
>> >>>
>> >>> https://dab1nmslvvntp.cloudfront.net/images/blogs/design/8bit-trans.png
>> >>>
>> >>> ffmpeg -i 8bit-trans.png -vf negate out.bmp
>> >>
>> >> i assume fate doesnt cover this yet, so a new fate test probably makes
>> >> sense
>> >
>> > i still think it would be more ideal if this is fully fixed
>> > (by alpha / non alpha pal8) or other.
>> >
>> > Because as is we have a set of bugs, and with this patchset we fix some
>> > while
>> > introducing other (maybe less important though) bugs.
>> > Then later behavior changes again when this is all fixed.
>> > A smaller number of behavior changes should be better and less confusing
>> > to
>> > users.
>>
>> I will rework the series, we can postpone actually fixing ffmpeg_filters.c
>>
>> and avfiltergraph.c pick_format(), I will add FIXME-s for those.
>
> At this point, actually adding a separate PAL8 format seems most
> attractive. Wouldn't it make everyone happy? Though I wonder which
> codecs are supposed to use it. (And if it's not clear whether something
> has alpha or not, we should strictly opt not to lose information,
> anyway.)

AFAIK all pal8 usecases have alpha, or set to opaque, Carl sent numerous such
patches.