| Message ID | 20160914210903.57936-1-vittorio.giovara@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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
> 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
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
> 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
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 --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);
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(+)