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

Submitted by Marton Balint on April 19, 2018, 7:32 p.m.

Details

Message ID 20180419193221.21712-2-cus@passwd.hu
State New
Headers show

Commit Message

Marton Balint April 19, 2018, 7:32 p.m.
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@passwd.hu>
---
 libavfilter/avfiltergraph.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Michael Niedermayer April 19, 2018, 11:07 p.m.
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@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.

Doesnt this patchset replace a problem by another problem ?
It may be better to not rush this and find a complete solution

a 2nd pixfmt and or scaning the 256 entries for their alpha values
is maybe the direction we could go


[...]
Marton Balint April 22, 2018, 11:44 a.m.
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@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...

>
> Doesnt this patchset replace a problem by another problem ?
> It may be better to not rush this and find a complete solution
>
> a 2nd pixfmt and or scaning the 256 entries for their alpha values
> is maybe the direction we could go

I don't think scaning can work for anything other than still images, as 
the palette can change per-frame AFAIK. Introducing a new pixel format 
seems the better approach, however keeping in mind compatibility and 
existing code, I'd rather make the newly introduced one the one without 
alpha, and move things gradually to it. Still seems like a fair amount of 
work...

Regards,
Marton
wm4 April 22, 2018, 12:46 p.m.
On Sun, 22 Apr 2018 13:44:19 +0200 (CEST)
Marton Balint <cus@passwd.hu> 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@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...
> 
> >
> > Doesnt this patchset replace a problem by another problem ?
> > It may be better to not rush this and find a complete solution
> >
> > a 2nd pixfmt and or scaning the 256 entries for their alpha values
> > is maybe the direction we could go  
> 
> I don't think scaning can work for anything other than still images, as 
> the palette can change per-frame AFAIK. Introducing a new pixel format 
> seems the better approach, however keeping in mind compatibility and 
> existing code, I'd rather make the newly introduced one the one without 
> alpha, and move things gradually to it. Still seems like a fair amount of 
> work...

PAL8 has the same problem as RGBA formats: you can't know whether it's
going to use alpha or not. (Indeed, there are codecs which can update
the palette, and who says filters won't change the palette?) With PAL8
it's just faster to verify for a particular frame. But in general, you
have to know in advance, and have to signal it in advance. I think that
works relatively well with PIXFMT_RGBA vs. PIXFMT_RGB0.

So I would argue it's fine to change PAL8 to having alpha, and if we
find cases that would really benefit from it, add PIXFMT_PAL8_RGB0. (Or
_RGBX or _NO_ALPHA or whatever.)
Michael Niedermayer April 23, 2018, 12:35 a.m.
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@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 ? 


[...]
Marton Balint April 23, 2018, 2:50 a.m.
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@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

Regards,
Marton
Michael Niedermayer April 24, 2018, 12:10 a.m.
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@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

thx

[...]
Michael Niedermayer April 24, 2018, 12:37 a.m.
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@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.

thx

[...]
Marton Balint April 24, 2018, 6:23 p.m.
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@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
wm4 April 24, 2018, 6:47 p.m.
On Tue, 24 Apr 2018 20:23:43 +0200 (CEST)
Marton Balint <cus@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@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.)
Paul B Mahol April 24, 2018, 6:50 p.m.
On 4/24/18, wm4 <nfxjfg@googlemail.com> wrote:
> On Tue, 24 Apr 2018 20:23:43 +0200 (CEST)
> Marton Balint <cus@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@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.

Patch hide | download patch | download mbox

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);
             enum AVPixelFormat best= AV_PIX_FMT_NONE;
             int i;
             for (i=0; i<link->in_formats->nb_formats; i++) {