diff mbox

[FFmpeg-devel,1/2] vf_colorspace: Interpret unspecified color range as limited range

Message ID 20160914210903.57936-1-vittorio.giovara@gmail.com
State Superseded
Headers show

Commit Message

Vittorio Giovara Sept. 14, 2016, 9:09 p.m. UTC
This is the assumption that is made in pixel format conversion do
throughout the code (in particular swscale and vf_colormatrix).

Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com>
---
 libavfilter/vf_colorspace.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Ronald S. Bultje Sept. 15, 2016, 12:03 p.m. UTC | #1
Hi,

On Wed, Sep 14, 2016 at 5:09 PM, Vittorio Giovara <
vittorio.giovara@gmail.com> wrote:

> This is the assumption that is made in pixel format conversion do
> throughout the code (in particular swscale and vf_colormatrix).
>
> Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com>
> ---
>  libavfilter/vf_colorspace.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c
> index b9ecb5f..7e87cd8 100644
> --- a/libavfilter/vf_colorspace.c
> +++ b/libavfilter/vf_colorspace.c
> @@ -522,6 +522,7 @@ static int get_range_off(int *off, int *y_rng, int
> *uv_rng,
>                           enum AVColorRange rng, int depth)
>  {
>      switch (rng) {
> +    case AVCOL_RANGE_UNSPECIFIED:
>      case AVCOL_RANGE_MPEG:
>          *off = 16 << (depth - 8);
>          *y_rng = 219 << (depth - 8);
> --
> 2.9.3


I'm open to this, but let me explain why I didn't follow the convention
here: the convention is wrong, or rather, the convention is outdated.

Classically, for low-resolution video, this is indeed true. But for modern
video (bt2020 etc., probably even smpte*) at larger resolutions, I feel the
default should be pc/jpeg range, not tv/mpeg range, because that's usually
what it is. I was somewhat tempted to not make automatic decisions that
could have a fairly substantial negative effect on the end result (without
informing the end user about it!). Asking the user to explicitly specify
the pixel range is one way of informing.

(You could also add a av_log(.., WARNING, ..) if the rng is UNSPECIFIED.)

But if others think this is the best solution, I'm OK with it.

Ronald
Vittorio Giovara Sept. 15, 2016, 12:33 p.m. UTC | #2
> Hi, On Wed, Sep 14, 2016 at 5:09 PM, Vittorio Giovara < vittorio.giovara at gmail.com> wrote: > This is the assumption that is made in pixel format conversion do > throughout the code (in particular swscale and vf_colormatrix). > > Signed-off-by: Vittorio Giovara <vittorio.giovara at gmail.com> > --- > libavfilter/vf_colorspace.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c > index b9ecb5f..7e87cd8 100644 > --- a/libavfilter/vf_colorspace.c > +++ b/libavfilter/vf_colorspace.c > @@ -522,6 +522,7 @@ static int get_range_off(int *off, int *y_rng, int > *uv_rng, > enum AVColorRange rng, int depth) > { > switch (rng) { > + case AVCOL_RANGE_UNSPECIFIED: >      case AVCOL_RANGE_MPEG: > *off = 16 << (depth - 8); > *y_rng = 219 << (depth - 8); > -- > 2.9.3   I'm open to this, but let me explain why I didn't follow the convention here: the convention is wrong, or rather, the convention is outdated. Classically, for low-resolution video, this is indeed true. But for modern video (bt2020 etc., probably even smpte*) at larger resolutions, I feel the default should be pc/jpeg range, not tv/mpeg range, because that's usually what it is.
Hi Ronald,
I am not sure about this point, unless I am missing something.

I've checked on the bt2020 spec and the nominal range described in table 5 shows 64-960 for 10bit which would match the limited representation.

Also bt2100 says that "full range" is "newly introduced" and should not be used unless "all parties agree", meaning range needs of be explicitly marked as full.
> I was somewhat tempted to not make automatic decisions that could have a fairly substantial negative effect on the end result (without informing the end user about it!). Asking the user to explicitly specify the pixel range is one way of informing. (You could also add a av_log(.., WARNING, ..) if the rng is UNSPECIFIED.) But if others think this is the best solution, I'm OK with it. Ronald
I agree we should always be very very careful about automatic assignments, although this one seems pretty safe.

I don't think it's necessary, but if you prefer I can add a warning whenever range is not specified (even though it might trigger a few false positives).

Cheers,
Vittorio
Ronald S. Bultje Sept. 15, 2016, 12:45 p.m. UTC | #3
Hi Vittorio,

On Thu, Sep 15, 2016 at 8:33 AM, Vittorio Giovara <
vittorio.giovara@gmail.com> wrote:

> I agree we should always be very very careful about automatic assignments,
> although this one seems pretty safe.
>
> I don't think it's necessary, but if you prefer I can add a warning
> whenever range is not specified (even though it might trigger a few false
> positives).


Which false positive are you afraid of?

Ronald
Vittorio Giovara Sept. 15, 2016, 1:32 p.m. UTC | #4
> On Sep 15, 2016, at 2:45 PM, Ronald S. Bultje <rsbultje@gmail.com> wrote:
> 
> Hi Vittorio,
> 
>> On Thu, Sep 15, 2016 at 8:33 AM, Vittorio Giovara <vittorio.giovara@gmail.com> wrote:
>> I agree we should always be very very careful about automatic assignments, although this one seems pretty safe.
>> 
>> I don't think it's necessary, but if you prefer I can add a warning whenever range is not specified (even though it might trigger a few false positives).
> 
> Which false positive are you afraid of?

By "false positive" I mean only the fact that it might needlessly print a warning, and the user might be confused by it.
Vittorio
Ronald S. Bultje Sept. 15, 2016, 1:38 p.m. UTC | #5
Hi,

On Thu, Sep 15, 2016 at 9:32 AM, Vittorio Giovara <
vittorio.giovara@gmail.com> wrote:

>
> On Sep 15, 2016, at 2:45 PM, Ronald S. Bultje <rsbultje@gmail.com> wrote:
>
> Hi Vittorio,
>
> On Thu, Sep 15, 2016 at 8:33 AM, Vittorio Giovara <
> vittorio.giovara@gmail.com> wrote:
>
>> I agree we should always be very very careful about automatic
>> assignments, although this one seems pretty safe.
>>
>> I don't think it's necessary, but if you prefer I can add a warning
>> whenever range is not specified (even though it might trigger a few false
>> positives).
>
>
> Which false positive are you afraid of?
>
>
> By "false positive" I mean only the fact that it might needlessly print a
> warning, and the user might be confused by it.
>

Right, but only because the range was not set. I'd like to get to the ideal
end situation where we encourage everyone (users, decoders, etc.) to always
explicitly set the range.

Aside, is it correct that if the pix_fmt was set to a YUVJ4xx variant,
range is automatically set to _JPEG?

Ronald
diff mbox

Patch

diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c
index b9ecb5f..7e87cd8 100644
--- a/libavfilter/vf_colorspace.c
+++ b/libavfilter/vf_colorspace.c
@@ -522,6 +522,7 @@  static int get_range_off(int *off, int *y_rng, int *uv_rng,
                          enum AVColorRange rng, int depth)
 {
     switch (rng) {
+    case AVCOL_RANGE_UNSPECIFIED:
     case AVCOL_RANGE_MPEG:
         *off = 16 << (depth - 8);
         *y_rng = 219 << (depth - 8);