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

[Marton Balint]

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.

Regards,
Marton