diff mbox

[FFmpeg-devel] avfilter/vf_scale: Do not set YUV color range for RGB formats

Message ID 20180320225946.12956-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer March 20, 2018, 10:59 p.m. UTC 
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavfilter/vf_scale.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Paul B Mahol March 21, 2018, 8:18 a.m. UTC | #1 
On 3/20/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavfilter/vf_scale.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
> index 9f45032e85..2f6fa4791d 100644
> --- a/libavfilter/vf_scale.c
> +++ b/libavfilter/vf_scale.c
> @@ -407,6 +407,7 @@ static int filter_frame(AVFilterLink *link, AVFrame *in)
>      AVFilterLink *outlink = link->dst->outputs[0];
>      AVFrame *out;
>      const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(link->format);
> +    const AVPixFmtDescriptor *out_desc =
> av_pix_fmt_desc_get(outlink->format);
>      char buf[32];
>      int in_range;
>
> @@ -497,7 +498,11 @@ static int filter_frame(AVFilterLink *link, AVFrame
> *in)
>                                       table, out_full,
>                                       brightness, contrast, saturation);
>
> -        out->color_range = out_full ? AVCOL_RANGE_JPEG : AVCOL_RANGE_MPEG;
> +        // color_range describes YUV, it is undefined for RGB
> +        if ((out_desc->flags & AV_PIX_FMT_FLAG_RGB) &&
> out_desc->nb_components != 1) {
> +            out->color_range = AVCOL_RANGE_UNSPECIFIED;
> +        } else
> +            out->color_range = out_full ? AVCOL_RANGE_JPEG :
> AVCOL_RANGE_MPEG;
>      }
>
>      av_reduce(&out->sample_aspect_ratio.num, &out->sample_aspect_ratio.den,
> --
> 2.16.2
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

This is not optimal, as full color_range should remain full when not changed.
wm4 March 21, 2018, 2:04 p.m. UTC | #2 
On Tue, 20 Mar 2018 23:59:46 +0100
Michael Niedermayer <michael@niedermayer.cc> wrote:

> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavfilter/vf_scale.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
> index 9f45032e85..2f6fa4791d 100644
> --- a/libavfilter/vf_scale.c
> +++ b/libavfilter/vf_scale.c
> @@ -407,6 +407,7 @@ static int filter_frame(AVFilterLink *link, AVFrame *in)
>      AVFilterLink *outlink = link->dst->outputs[0];
>      AVFrame *out;
>      const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(link->format);
> +    const AVPixFmtDescriptor *out_desc = av_pix_fmt_desc_get(outlink->format);
>      char buf[32];
>      int in_range;
>  
> @@ -497,7 +498,11 @@ static int filter_frame(AVFilterLink *link, AVFrame *in)
>                                       table, out_full,
>                                       brightness, contrast, saturation);
>  
> -        out->color_range = out_full ? AVCOL_RANGE_JPEG : AVCOL_RANGE_MPEG;
> +        // color_range describes YUV, it is undefined for RGB
> +        if ((out_desc->flags & AV_PIX_FMT_FLAG_RGB) && out_desc->nb_components != 1) {
> +            out->color_range = AVCOL_RANGE_UNSPECIFIED;
> +        } else
> +            out->color_range = out_full ? AVCOL_RANGE_JPEG : AVCOL_RANGE_MPEG;
>      }
>  
>      av_reduce(&out->sample_aspect_ratio.num, &out->sample_aspect_ratio.den,

Why is it not setting full range for gray? PNGs can use this for
grayscale images, so they would be affected by this too.
Michael Niedermayer March 25, 2018, 11:55 p.m. UTC | #3 
On Wed, Mar 21, 2018 at 03:04:35PM +0100, wm4 wrote:
> On Tue, 20 Mar 2018 23:59:46 +0100
> Michael Niedermayer <michael@niedermayer.cc> wrote:
> 
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavfilter/vf_scale.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
> > index 9f45032e85..2f6fa4791d 100644
> > --- a/libavfilter/vf_scale.c
> > +++ b/libavfilter/vf_scale.c
> > @@ -407,6 +407,7 @@ static int filter_frame(AVFilterLink *link, AVFrame *in)
> >      AVFilterLink *outlink = link->dst->outputs[0];
> >      AVFrame *out;
> >      const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(link->format);
> > +    const AVPixFmtDescriptor *out_desc = av_pix_fmt_desc_get(outlink->format);
> >      char buf[32];
> >      int in_range;
> >  
> > @@ -497,7 +498,11 @@ static int filter_frame(AVFilterLink *link, AVFrame *in)
> >                                       table, out_full,
> >                                       brightness, contrast, saturation);
> >  
> > -        out->color_range = out_full ? AVCOL_RANGE_JPEG : AVCOL_RANGE_MPEG;
> > +        // color_range describes YUV, it is undefined for RGB
> > +        if ((out_desc->flags & AV_PIX_FMT_FLAG_RGB) && out_desc->nb_components != 1) {
> > +            out->color_range = AVCOL_RANGE_UNSPECIFIED;
> > +        } else
> > +            out->color_range = out_full ? AVCOL_RANGE_JPEG : AVCOL_RANGE_MPEG;
> >      }
> >  
> >      av_reduce(&out->sample_aspect_ratio.num, &out->sample_aspect_ratio.den,
> 
> Why is it not setting full range for gray? PNGs can use this for
> grayscale images, so they would be affected by this too.

I do not understand what you describe here. Please describe the issue
clearer and or provide a testcase.

there is no unique gray pixel format.
gray8 is not a AV_PIX_FMT_FLAG_RGB format, thus its treated the same as
before this patch



[...]
Michael Niedermayer March 26, 2018, midnight UTC | #4 
On Wed, Mar 21, 2018 at 09:18:21AM +0100, Paul B Mahol wrote:
> On 3/20/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavfilter/vf_scale.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
> > index 9f45032e85..2f6fa4791d 100644
> > --- a/libavfilter/vf_scale.c
> > +++ b/libavfilter/vf_scale.c
> > @@ -407,6 +407,7 @@ static int filter_frame(AVFilterLink *link, AVFrame *in)
> >      AVFilterLink *outlink = link->dst->outputs[0];
> >      AVFrame *out;
> >      const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(link->format);
> > +    const AVPixFmtDescriptor *out_desc =
> > av_pix_fmt_desc_get(outlink->format);
> >      char buf[32];
> >      int in_range;
> >
> > @@ -497,7 +498,11 @@ static int filter_frame(AVFilterLink *link, AVFrame
> > *in)
> >                                       table, out_full,
> >                                       brightness, contrast, saturation);
> >
> > -        out->color_range = out_full ? AVCOL_RANGE_JPEG : AVCOL_RANGE_MPEG;
> > +        // color_range describes YUV, it is undefined for RGB
> > +        if ((out_desc->flags & AV_PIX_FMT_FLAG_RGB) &&
> > out_desc->nb_components != 1) {
> > +            out->color_range = AVCOL_RANGE_UNSPECIFIED;
> > +        } else
> > +            out->color_range = out_full ? AVCOL_RANGE_JPEG :
> > AVCOL_RANGE_MPEG;
> >      }
> >
> >      av_reduce(&out->sample_aspect_ratio.num, &out->sample_aspect_ratio.den,
> > --
> > 2.16.2
> >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> 
> This is not optimal, as full color_range should remain full when not changed.

there is no range for rgb formats. The range is specific to YUV based
formats. 
Thats, if iam guessing correctly what you meant. You did not really say
which case you meant here. So maybe there is an issue and i misunderstand
what you refer to

[...]
Paul B Mahol March 26, 2018, 6:34 a.m. UTC | #5 
On 3/26/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Wed, Mar 21, 2018 at 09:18:21AM +0100, Paul B Mahol wrote:
>> On 3/20/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
>> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> > ---
>> >  libavfilter/vf_scale.c | 7 ++++++-
>> >  1 file changed, 6 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
>> > index 9f45032e85..2f6fa4791d 100644
>> > --- a/libavfilter/vf_scale.c
>> > +++ b/libavfilter/vf_scale.c
>> > @@ -407,6 +407,7 @@ static int filter_frame(AVFilterLink *link, AVFrame
>> > *in)
>> >      AVFilterLink *outlink = link->dst->outputs[0];
>> >      AVFrame *out;
>> >      const AVPixFmtDescriptor *desc =
>> > av_pix_fmt_desc_get(link->format);
>> > +    const AVPixFmtDescriptor *out_desc =
>> > av_pix_fmt_desc_get(outlink->format);
>> >      char buf[32];
>> >      int in_range;
>> >
>> > @@ -497,7 +498,11 @@ static int filter_frame(AVFilterLink *link,
>> > AVFrame
>> > *in)
>> >                                       table, out_full,
>> >                                       brightness, contrast,
>> > saturation);
>> >
>> > -        out->color_range = out_full ? AVCOL_RANGE_JPEG :
>> > AVCOL_RANGE_MPEG;
>> > +        // color_range describes YUV, it is undefined for RGB
>> > +        if ((out_desc->flags & AV_PIX_FMT_FLAG_RGB) &&
>> > out_desc->nb_components != 1) {
>> > +            out->color_range = AVCOL_RANGE_UNSPECIFIED;
>> > +        } else
>> > +            out->color_range = out_full ? AVCOL_RANGE_JPEG :
>> > AVCOL_RANGE_MPEG;
>> >      }
>> >
>> >      av_reduce(&out->sample_aspect_ratio.num,
>> > &out->sample_aspect_ratio.den,
>> > --
>> > 2.16.2
>> >
>> > _______________________________________________
>> > ffmpeg-devel mailing list
>> > ffmpeg-devel@ffmpeg.org
>> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> >
>>
>> This is not optimal, as full color_range should remain full when not
>> changed.
>
> there is no range for rgb formats. The range is specific to YUV based
> formats.
> Thats, if iam guessing correctly what you meant. You did not really say
> which case you meant here. So maybe there is an issue and i misunderstand
> what you refer to

Maybe not for swscale here but does exist otherwise.

>
> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Many things microsoft did are stupid, but not doing something just because
> microsoft did it is even more stupid. If everything ms did were stupid they
> would be bankrupt already.
>
Martin Vignali March 26, 2018, 10:21 a.m. UTC | #6 
> >> This is not optimal, as full color_range should remain full when not
> >> changed.
> >
> > there is no range for rgb formats. The range is specific to YUV based
> > formats.
> > Thats, if iam guessing correctly what you meant. You did not really say
> > which case you meant here. So maybe there is an issue and i misunderstand
> > what you refer to
>
> Maybe not for swscale here but does exist otherwise.
>
> What software use limited for rgb ?

I understand for gray (it can be gray of yuv data (limited or full) or gray
for RGB data)),
but for RGB/RGBA, never see limited (and doesn't see the interest :-)

Martin
Paul B Mahol March 26, 2018, 10:50 a.m. UTC | #7 
On 3/26/18, Martin Vignali <martin.vignali@gmail.com> wrote:
>> >> This is not optimal, as full color_range should remain full when not
>> >> changed.
>> >
>> > there is no range for rgb formats. The range is specific to YUV based
>> > formats.
>> > Thats, if iam guessing correctly what you meant. You did not really say
>> > which case you meant here. So maybe there is an issue and i
>> > misunderstand
>> > what you refer to
>>
>> Maybe not for swscale here but does exist otherwise.
>>
>> What software use limited for rgb ?
>
> I understand for gray (it can be gray of yuv data (limited or full) or gray
> for RGB data)),
> but for RGB/RGBA, never see limited (and doesn't see the interest :-)
>
> Martin
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

OK then, just apply patch.
Michael Niedermayer March 26, 2018, 9:44 p.m. UTC | #8 
On Mon, Mar 26, 2018 at 08:34:06AM +0200, Paul B Mahol wrote:
> On 3/26/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > On Wed, Mar 21, 2018 at 09:18:21AM +0100, Paul B Mahol wrote:
> >> On 3/20/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >> > ---
> >> >  libavfilter/vf_scale.c | 7 ++++++-
> >> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
> >> > index 9f45032e85..2f6fa4791d 100644
> >> > --- a/libavfilter/vf_scale.c
> >> > +++ b/libavfilter/vf_scale.c
> >> > @@ -407,6 +407,7 @@ static int filter_frame(AVFilterLink *link, AVFrame
> >> > *in)
> >> >      AVFilterLink *outlink = link->dst->outputs[0];
> >> >      AVFrame *out;
> >> >      const AVPixFmtDescriptor *desc =
> >> > av_pix_fmt_desc_get(link->format);
> >> > +    const AVPixFmtDescriptor *out_desc =
> >> > av_pix_fmt_desc_get(outlink->format);
> >> >      char buf[32];
> >> >      int in_range;
> >> >
> >> > @@ -497,7 +498,11 @@ static int filter_frame(AVFilterLink *link,
> >> > AVFrame
> >> > *in)
> >> >                                       table, out_full,
> >> >                                       brightness, contrast,
> >> > saturation);
> >> >
> >> > -        out->color_range = out_full ? AVCOL_RANGE_JPEG :
> >> > AVCOL_RANGE_MPEG;
> >> > +        // color_range describes YUV, it is undefined for RGB
> >> > +        if ((out_desc->flags & AV_PIX_FMT_FLAG_RGB) &&
> >> > out_desc->nb_components != 1) {
> >> > +            out->color_range = AVCOL_RANGE_UNSPECIFIED;
> >> > +        } else
> >> > +            out->color_range = out_full ? AVCOL_RANGE_JPEG :
> >> > AVCOL_RANGE_MPEG;
> >> >      }
> >> >
> >> >      av_reduce(&out->sample_aspect_ratio.num,
> >> > &out->sample_aspect_ratio.den,
> >> > --
> >> > 2.16.2
> >> >
> >> > _______________________________________________
> >> > ffmpeg-devel mailing list
> >> > ffmpeg-devel@ffmpeg.org
> >> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >> >
> >>
> >> This is not optimal, as full color_range should remain full when not
> >> changed.
> >
> > there is no range for rgb formats. The range is specific to YUV based
> > formats.
> > Thats, if iam guessing correctly what you meant. You did not really say
> > which case you meant here. So maybe there is an issue and i misunderstand
> > what you refer to
> 
> Maybe not for swscale here but does exist otherwise.

It has no meaning for RGB. There is no limited range RGB
also, the API defines it only for YUV. So if it had meaning for RGB its not defined
what meaning that would be. The ranges for luma and chroma differ so its not
natural or obvious how to extend it to RGB.
Iam also not aware of any specification that defines limited range RGB

also please be a little more verbose about what you speak of. That should
speed up resolving this

/**
 * MPEG vs JPEG YUV range.
 */
enum AVColorRange {
    AVCOL_RANGE_UNSPECIFIED = 0,
    AVCOL_RANGE_MPEG        = 1, ///< the normal 219*2^(n-8) "MPEG" YUV ranges
    AVCOL_RANGE_JPEG        = 2, ///< the normal     2^n-1   "JPEG" YUV ranges
    AVCOL_RANGE_NB               ///< Not part of ABI


[...]
Michael Niedermayer March 26, 2018, 9:46 p.m. UTC | #9 
On Mon, Mar 26, 2018 at 12:50:41PM +0200, Paul B Mahol wrote:
> On 3/26/18, Martin Vignali <martin.vignali@gmail.com> wrote:
> >> >> This is not optimal, as full color_range should remain full when not
> >> >> changed.
> >> >
> >> > there is no range for rgb formats. The range is specific to YUV based
> >> > formats.
> >> > Thats, if iam guessing correctly what you meant. You did not really say
> >> > which case you meant here. So maybe there is an issue and i
> >> > misunderstand
> >> > what you refer to
> >>
> >> Maybe not for swscale here but does exist otherwise.
> >>
> >> What software use limited for rgb ?
> >
> > I understand for gray (it can be gray of yuv data (limited or full) or gray
> > for RGB data)),
> > but for RGB/RGBA, never see limited (and doesn't see the interest :-)
> >
> > Martin
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> 

> OK then, just apply patch.

ok, will apply

thanks!

[...]
Mark Thompson March 26, 2018, 9:56 p.m. UTC | #10 
On 26/03/18 22:44, Michael Niedermayer wrote:
> On Mon, Mar 26, 2018 at 08:34:06AM +0200, Paul B Mahol wrote:
>> On 3/26/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
>>> On Wed, Mar 21, 2018 at 09:18:21AM +0100, Paul B Mahol wrote:
>>>> On 3/20/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>>> ---
>>>>>  libavfilter/vf_scale.c | 7 ++++++-
>>>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
>>>>> index 9f45032e85..2f6fa4791d 100644
>>>>> --- a/libavfilter/vf_scale.c
>>>>> +++ b/libavfilter/vf_scale.c
>>>>> @@ -407,6 +407,7 @@ static int filter_frame(AVFilterLink *link, AVFrame
>>>>> *in)
>>>>>      AVFilterLink *outlink = link->dst->outputs[0];
>>>>>      AVFrame *out;
>>>>>      const AVPixFmtDescriptor *desc =
>>>>> av_pix_fmt_desc_get(link->format);
>>>>> +    const AVPixFmtDescriptor *out_desc =
>>>>> av_pix_fmt_desc_get(outlink->format);
>>>>>      char buf[32];
>>>>>      int in_range;
>>>>>
>>>>> @@ -497,7 +498,11 @@ static int filter_frame(AVFilterLink *link,
>>>>> AVFrame
>>>>> *in)
>>>>>                                       table, out_full,
>>>>>                                       brightness, contrast,
>>>>> saturation);
>>>>>
>>>>> -        out->color_range = out_full ? AVCOL_RANGE_JPEG :
>>>>> AVCOL_RANGE_MPEG;
>>>>> +        // color_range describes YUV, it is undefined for RGB
>>>>> +        if ((out_desc->flags & AV_PIX_FMT_FLAG_RGB) &&
>>>>> out_desc->nb_components != 1) {
>>>>> +            out->color_range = AVCOL_RANGE_UNSPECIFIED;
>>>>> +        } else
>>>>> +            out->color_range = out_full ? AVCOL_RANGE_JPEG :
>>>>> AVCOL_RANGE_MPEG;
>>>>>      }
>>>>>
>>>>>      av_reduce(&out->sample_aspect_ratio.num,
>>>>> &out->sample_aspect_ratio.den,
>>>>> --
>>>>> 2.16.2
>>>>>
>>>>
>>>> This is not optimal, as full color_range should remain full when not
>>>> changed.
>>>
>>> there is no range for rgb formats. The range is specific to YUV based
>>> formats.
>>> Thats, if iam guessing correctly what you meant. You did not really say
>>> which case you meant here. So maybe there is an issue and i misunderstand
>>> what you refer to
>>
>> Maybe not for swscale here but does exist otherwise.
> 
> It has no meaning for RGB. There is no limited range RGB
> also, the API defines it only for YUV. So if it had meaning for RGB its not defined
> what meaning that would be. The ranges for luma and chroma differ so its not
> natural or obvious how to extend it to RGB.
> Iam also not aware of any specification that defines limited range RGB
See HDMI, HD-SDI, CEA-861, pretty much anything related to sending RGB over wires in a non-PC context.

(No comment on whether it is useful to include it, but dismissing limited range RGB as not a thing is probably not a good idea.  I would kindof expect things like DeckLink to be using limited-range RGB everywhere, but I'm not familiar with the details there.)

- Mark
Michael Niedermayer March 26, 2018, 10:23 p.m. UTC | #11 
On Mon, Mar 26, 2018 at 10:56:47PM +0100, Mark Thompson wrote:
> On 26/03/18 22:44, Michael Niedermayer wrote:
> > On Mon, Mar 26, 2018 at 08:34:06AM +0200, Paul B Mahol wrote:
> >> On 3/26/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> >>> On Wed, Mar 21, 2018 at 09:18:21AM +0100, Paul B Mahol wrote:
> >>>> On 3/20/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> >>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >>>>> ---
> >>>>>  libavfilter/vf_scale.c | 7 ++++++-
> >>>>>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
> >>>>> index 9f45032e85..2f6fa4791d 100644
> >>>>> --- a/libavfilter/vf_scale.c
> >>>>> +++ b/libavfilter/vf_scale.c
> >>>>> @@ -407,6 +407,7 @@ static int filter_frame(AVFilterLink *link, AVFrame
> >>>>> *in)
> >>>>>      AVFilterLink *outlink = link->dst->outputs[0];
> >>>>>      AVFrame *out;
> >>>>>      const AVPixFmtDescriptor *desc =
> >>>>> av_pix_fmt_desc_get(link->format);
> >>>>> +    const AVPixFmtDescriptor *out_desc =
> >>>>> av_pix_fmt_desc_get(outlink->format);
> >>>>>      char buf[32];
> >>>>>      int in_range;
> >>>>>
> >>>>> @@ -497,7 +498,11 @@ static int filter_frame(AVFilterLink *link,
> >>>>> AVFrame
> >>>>> *in)
> >>>>>                                       table, out_full,
> >>>>>                                       brightness, contrast,
> >>>>> saturation);
> >>>>>
> >>>>> -        out->color_range = out_full ? AVCOL_RANGE_JPEG :
> >>>>> AVCOL_RANGE_MPEG;
> >>>>> +        // color_range describes YUV, it is undefined for RGB
> >>>>> +        if ((out_desc->flags & AV_PIX_FMT_FLAG_RGB) &&
> >>>>> out_desc->nb_components != 1) {
> >>>>> +            out->color_range = AVCOL_RANGE_UNSPECIFIED;
> >>>>> +        } else
> >>>>> +            out->color_range = out_full ? AVCOL_RANGE_JPEG :
> >>>>> AVCOL_RANGE_MPEG;
> >>>>>      }
> >>>>>
> >>>>>      av_reduce(&out->sample_aspect_ratio.num,
> >>>>> &out->sample_aspect_ratio.den,
> >>>>> --
> >>>>> 2.16.2
> >>>>>
> >>>>
> >>>> This is not optimal, as full color_range should remain full when not
> >>>> changed.
> >>>
> >>> there is no range for rgb formats. The range is specific to YUV based
> >>> formats.
> >>> Thats, if iam guessing correctly what you meant. You did not really say
> >>> which case you meant here. So maybe there is an issue and i misunderstand
> >>> what you refer to
> >>
> >> Maybe not for swscale here but does exist otherwise.
> > 
> > It has no meaning for RGB. There is no limited range RGB
> > also, the API defines it only for YUV. So if it had meaning for RGB its not defined
> > what meaning that would be. The ranges for luma and chroma differ so its not
> > natural or obvious how to extend it to RGB.
> > Iam also not aware of any specification that defines limited range RGB
> See HDMI, HD-SDI, CEA-861, pretty much anything related to sending RGB over wires in a non-PC context.
> 
> (No comment on whether it is useful to include it, but dismissing limited range RGB as not a thing is probably not a good idea.  I would kindof expect things like DeckLink to be using limited-range RGB everywhere, but I'm not familiar with the details there.)

interresting, i was unaware of this
are these specs publically available ? i could find CEA-861 but not the others
quickly

thx


[...]
Michael Niedermayer March 27, 2018, 10:14 a.m. UTC | #12 
On Mon, Mar 26, 2018 at 10:56:47PM +0100, Mark Thompson wrote:
> On 26/03/18 22:44, Michael Niedermayer wrote:
> > On Mon, Mar 26, 2018 at 08:34:06AM +0200, Paul B Mahol wrote:
> >> On 3/26/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> >>> On Wed, Mar 21, 2018 at 09:18:21AM +0100, Paul B Mahol wrote:
> >>>> On 3/20/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> >>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >>>>> ---
> >>>>>  libavfilter/vf_scale.c | 7 ++++++-
> >>>>>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
> >>>>> index 9f45032e85..2f6fa4791d 100644
> >>>>> --- a/libavfilter/vf_scale.c
> >>>>> +++ b/libavfilter/vf_scale.c
> >>>>> @@ -407,6 +407,7 @@ static int filter_frame(AVFilterLink *link, AVFrame
> >>>>> *in)
> >>>>>      AVFilterLink *outlink = link->dst->outputs[0];
> >>>>>      AVFrame *out;
> >>>>>      const AVPixFmtDescriptor *desc =
> >>>>> av_pix_fmt_desc_get(link->format);
> >>>>> +    const AVPixFmtDescriptor *out_desc =
> >>>>> av_pix_fmt_desc_get(outlink->format);
> >>>>>      char buf[32];
> >>>>>      int in_range;
> >>>>>
> >>>>> @@ -497,7 +498,11 @@ static int filter_frame(AVFilterLink *link,
> >>>>> AVFrame
> >>>>> *in)
> >>>>>                                       table, out_full,
> >>>>>                                       brightness, contrast,
> >>>>> saturation);
> >>>>>
> >>>>> -        out->color_range = out_full ? AVCOL_RANGE_JPEG :
> >>>>> AVCOL_RANGE_MPEG;
> >>>>> +        // color_range describes YUV, it is undefined for RGB
> >>>>> +        if ((out_desc->flags & AV_PIX_FMT_FLAG_RGB) &&
> >>>>> out_desc->nb_components != 1) {
> >>>>> +            out->color_range = AVCOL_RANGE_UNSPECIFIED;
> >>>>> +        } else
> >>>>> +            out->color_range = out_full ? AVCOL_RANGE_JPEG :
> >>>>> AVCOL_RANGE_MPEG;
> >>>>>      }
> >>>>>
> >>>>>      av_reduce(&out->sample_aspect_ratio.num,
> >>>>> &out->sample_aspect_ratio.den,
> >>>>> --
> >>>>> 2.16.2
> >>>>>
> >>>>
> >>>> This is not optimal, as full color_range should remain full when not
> >>>> changed.
> >>>
> >>> there is no range for rgb formats. The range is specific to YUV based
> >>> formats.
> >>> Thats, if iam guessing correctly what you meant. You did not really say
> >>> which case you meant here. So maybe there is an issue and i misunderstand
> >>> what you refer to
> >>
> >> Maybe not for swscale here but does exist otherwise.
> > 
> > It has no meaning for RGB. There is no limited range RGB
> > also, the API defines it only for YUV. So if it had meaning for RGB its not defined
> > what meaning that would be. The ranges for luma and chroma differ so its not
> > natural or obvious how to extend it to RGB.
> > Iam also not aware of any specification that defines limited range RGB
> See HDMI, HD-SDI, CEA-861, pretty much anything related to sending RGB over wires in a non-PC context.
> 
> (No comment on whether it is useful to include it, but dismissing limited range RGB as not a thing is probably not a good idea.  I would kindof expect things like DeckLink to be using limited-range RGB everywhere, but I'm not familiar with the details there.)

Is this used just over wires or also in software ?
aka should we update our enum to also cover this and 
If yes then we also probably should add support for this
to swscale

thx

[...]
wm4 March 27, 2018, 3:04 p.m. UTC | #13 
On Tue, 27 Mar 2018 12:14:55 +0200
Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Mon, Mar 26, 2018 at 10:56:47PM +0100, Mark Thompson wrote:
> > On 26/03/18 22:44, Michael Niedermayer wrote:  
> > > On Mon, Mar 26, 2018 at 08:34:06AM +0200, Paul B Mahol wrote:  
> > >> On 3/26/18, Michael Niedermayer <michael@niedermayer.cc> wrote:  
> > >>> On Wed, Mar 21, 2018 at 09:18:21AM +0100, Paul B Mahol wrote:  
> > >>>> On 3/20/18, Michael Niedermayer <michael@niedermayer.cc> wrote:  
> > >>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > >>>>> ---
> > >>>>>  libavfilter/vf_scale.c | 7 ++++++-
> > >>>>>  1 file changed, 6 insertions(+), 1 deletion(-)
> > >>>>>
> > >>>>> diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
> > >>>>> index 9f45032e85..2f6fa4791d 100644
> > >>>>> --- a/libavfilter/vf_scale.c
> > >>>>> +++ b/libavfilter/vf_scale.c
> > >>>>> @@ -407,6 +407,7 @@ static int filter_frame(AVFilterLink *link, AVFrame
> > >>>>> *in)
> > >>>>>      AVFilterLink *outlink = link->dst->outputs[0];
> > >>>>>      AVFrame *out;
> > >>>>>      const AVPixFmtDescriptor *desc =
> > >>>>> av_pix_fmt_desc_get(link->format);
> > >>>>> +    const AVPixFmtDescriptor *out_desc =
> > >>>>> av_pix_fmt_desc_get(outlink->format);
> > >>>>>      char buf[32];
> > >>>>>      int in_range;
> > >>>>>
> > >>>>> @@ -497,7 +498,11 @@ static int filter_frame(AVFilterLink *link,
> > >>>>> AVFrame
> > >>>>> *in)
> > >>>>>                                       table, out_full,
> > >>>>>                                       brightness, contrast,
> > >>>>> saturation);
> > >>>>>
> > >>>>> -        out->color_range = out_full ? AVCOL_RANGE_JPEG :
> > >>>>> AVCOL_RANGE_MPEG;
> > >>>>> +        // color_range describes YUV, it is undefined for RGB
> > >>>>> +        if ((out_desc->flags & AV_PIX_FMT_FLAG_RGB) &&
> > >>>>> out_desc->nb_components != 1) {
> > >>>>> +            out->color_range = AVCOL_RANGE_UNSPECIFIED;
> > >>>>> +        } else
> > >>>>> +            out->color_range = out_full ? AVCOL_RANGE_JPEG :
> > >>>>> AVCOL_RANGE_MPEG;
> > >>>>>      }
> > >>>>>
> > >>>>>      av_reduce(&out->sample_aspect_ratio.num,
> > >>>>> &out->sample_aspect_ratio.den,
> > >>>>> --
> > >>>>> 2.16.2
> > >>>>>  
> > >>>>
> > >>>> This is not optimal, as full color_range should remain full when not
> > >>>> changed.  
> > >>>
> > >>> there is no range for rgb formats. The range is specific to YUV based
> > >>> formats.
> > >>> Thats, if iam guessing correctly what you meant. You did not really say
> > >>> which case you meant here. So maybe there is an issue and i misunderstand
> > >>> what you refer to  
> > >>
> > >> Maybe not for swscale here but does exist otherwise.  
> > > 
> > > It has no meaning for RGB. There is no limited range RGB
> > > also, the API defines it only for YUV. So if it had meaning for RGB its not defined
> > > what meaning that would be. The ranges for luma and chroma differ so its not
> > > natural or obvious how to extend it to RGB.
> > > Iam also not aware of any specification that defines limited range RGB  
> > See HDMI, HD-SDI, CEA-861, pretty much anything related to sending RGB over wires in a non-PC context.
> > 
> > (No comment on whether it is useful to include it, but dismissing limited range RGB as not a thing is probably not a good idea.  I would kindof expect things like DeckLink to be using limited-range RGB everywhere, but I'm not familiar with the details there.)  
> 
> Is this used just over wires or also in software ?

Yes, it can happen that you "need" to output limited RGB for consumer
burning trash fire hardware.

> aka should we update our enum to also cover this and 
> If yes then we also probably should add support for this
> to swscale


I tried to use it once and considered it a libswscale bug and reported
it, but was overruled or something.
diff mbox

Patch

diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
index 9f45032e85..2f6fa4791d 100644
--- a/libavfilter/vf_scale.c
+++ b/libavfilter/vf_scale.c
@@ -407,6 +407,7 @@  static int filter_frame(AVFilterLink *link, AVFrame *in)
     AVFilterLink *outlink = link->dst->outputs[0];
     AVFrame *out;
     const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(link->format);
+    const AVPixFmtDescriptor *out_desc = av_pix_fmt_desc_get(outlink->format);
     char buf[32];
     int in_range;
 
@@ -497,7 +498,11 @@  static int filter_frame(AVFilterLink *link, AVFrame *in)
                                      table, out_full,
                                      brightness, contrast, saturation);
 
-        out->color_range = out_full ? AVCOL_RANGE_JPEG : AVCOL_RANGE_MPEG;
+        // color_range describes YUV, it is undefined for RGB
+        if ((out_desc->flags & AV_PIX_FMT_FLAG_RGB) && out_desc->nb_components != 1) {
+            out->color_range = AVCOL_RANGE_UNSPECIFIED;
+        } else
+            out->color_range = out_full ? AVCOL_RANGE_JPEG : AVCOL_RANGE_MPEG;
     }
 
     av_reduce(&out->sample_aspect_ratio.num, &out->sample_aspect_ratio.den,