| Message ID | 20160916132049.26957-1-vittorio.giovara@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
Hi, On Fri, Sep 16, 2016 at 9:20 AM, 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 BT-specifications > mandate. > > Add a warning to inform the user that an automatic selection is being > made. > > Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com> > --- > libavfilter/vf_colorspace.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c > index e69be50..41d1692 100644 > --- a/libavfilter/vf_colorspace.c > +++ b/libavfilter/vf_colorspace.c > @@ -518,10 +518,13 @@ static int convert(AVFilterContext *ctx, void *data, > int job_nr, int n_jobs) > return 0; > } > > -static int get_range_off(int *off, int *y_rng, int *uv_rng, > +static int get_range_off(AVFilterContext *ctx, int *off, > + int *y_rng, int *uv_rng, > enum AVColorRange rng, int depth) > { > switch (rng) { > + case AVCOL_RANGE_UNSPECIFIED: > + av_log(ctx, AV_LOG_WARNING, "Input range not set, assuming > tv/mpeg\n"); > case AVCOL_RANGE_MPEG: > *off = 16 << (depth - 8); > *y_rng = 219 << (depth - 8); > @@ -740,7 +743,7 @@ static int create_filtergraph(AVFilterContext *ctx, > double rgb2yuv[3][3], (*yuv2rgb)[3] = s->yuv2rgb_dbl_coeffs; > int off, bits, in_rng; > > - res = get_range_off(&off, &s->in_y_rng, &s->in_uv_rng, > + res = get_range_off(ctx, &off, &s->in_y_rng, &s->in_uv_rng, > s->in_rng, in_desc->comp[0].depth); > if (res < 0) { > av_log(ctx, AV_LOG_ERROR, > @@ -773,7 +776,7 @@ static int create_filtergraph(AVFilterContext *ctx, > double (*rgb2yuv)[3] = s->rgb2yuv_dbl_coeffs; > int off, out_rng, bits; > > - res = get_range_off(&off, &s->out_y_rng, &s->out_uv_rng, > + res = get_range_off(ctx, &off, &s->out_y_rng, &s->out_uv_rng, > s->out_rng, out_desc->comp[0].depth); > if (res < 0) { > av_log(ctx, AV_LOG_ERROR, > -- > 2.9.3 OK. (I'll leave for a few hours for others to review, and apply after.) Ronald
On Fri, Sep 16, 2016 at 03:20:49PM +0200, Vittorio Giovara wrote: > This is the assumption that is made in pixel format conversion do > throughout the code (in particular swscale), and BT-specifications > mandate. > > Add a warning to inform the user that an automatic selection is being > made. > > Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com> > --- > libavfilter/vf_colorspace.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c > index e69be50..41d1692 100644 > --- a/libavfilter/vf_colorspace.c > +++ b/libavfilter/vf_colorspace.c > @@ -518,10 +518,13 @@ static int convert(AVFilterContext *ctx, void *data, int job_nr, int n_jobs) > return 0; > } > > -static int get_range_off(int *off, int *y_rng, int *uv_rng, > +static int get_range_off(AVFilterContext *ctx, int *off, > + int *y_rng, int *uv_rng, > enum AVColorRange rng, int depth) > { > switch (rng) { > + case AVCOL_RANGE_UNSPECIFIED: > + av_log(ctx, AV_LOG_WARNING, "Input range not set, assuming tv/mpeg\n"); nit: I think coverity likes a "// Fall-through" below this line in these cases. [...]
Hi, On Fri, Sep 16, 2016 at 9:27 AM, Clément Bœsch <u@pkh.me> wrote: > On Fri, Sep 16, 2016 at 03:20:49PM +0200, Vittorio Giovara wrote: > > This is the assumption that is made in pixel format conversion do > > throughout the code (in particular swscale), and BT-specifications > > mandate. > > > > Add a warning to inform the user that an automatic selection is being > > made. > > > > Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com> > > --- > > libavfilter/vf_colorspace.c | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c > > index e69be50..41d1692 100644 > > --- a/libavfilter/vf_colorspace.c > > +++ b/libavfilter/vf_colorspace.c > > @@ -518,10 +518,13 @@ static int convert(AVFilterContext *ctx, void > *data, int job_nr, int n_jobs) > > return 0; > > } > > > > -static int get_range_off(int *off, int *y_rng, int *uv_rng, > > +static int get_range_off(AVFilterContext *ctx, int *off, > > + int *y_rng, int *uv_rng, > > enum AVColorRange rng, int depth) > > { > > switch (rng) { > > + case AVCOL_RANGE_UNSPECIFIED: > > + av_log(ctx, AV_LOG_WARNING, "Input range not set, assuming > tv/mpeg\n"); > > nit: I think coverity likes a "// Fall-through" below this line in these > cases. Pushed with comment. Ronald
On Mon, Sep 19, 2016 at 8:36 AM, Ronald S. Bultje <rsbultje@gmail.com> wrote: > Hi, > > On Fri, Sep 16, 2016 at 9:27 AM, Clément Bœsch <u@pkh.me> wrote: >> >> On Fri, Sep 16, 2016 at 03:20:49PM +0200, Vittorio Giovara wrote: >> > This is the assumption that is made in pixel format conversion do >> > throughout the code (in particular swscale), and BT-specifications >> > mandate. >> > >> > Add a warning to inform the user that an automatic selection is being >> > made. >> > >> > Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com> >> > --- >> > libavfilter/vf_colorspace.c | 9 ++++++--- >> > 1 file changed, 6 insertions(+), 3 deletions(-) >> > >> > diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c >> > index e69be50..41d1692 100644 >> > --- a/libavfilter/vf_colorspace.c >> > +++ b/libavfilter/vf_colorspace.c >> > @@ -518,10 +518,13 @@ static int convert(AVFilterContext *ctx, void >> > *data, int job_nr, int n_jobs) >> > return 0; >> > } >> > >> > -static int get_range_off(int *off, int *y_rng, int *uv_rng, >> > +static int get_range_off(AVFilterContext *ctx, int *off, >> > + int *y_rng, int *uv_rng, >> > enum AVColorRange rng, int depth) >> > { >> > switch (rng) { >> > + case AVCOL_RANGE_UNSPECIFIED: >> > + av_log(ctx, AV_LOG_WARNING, "Input range not set, assuming >> > tv/mpeg\n"); Hey Ronald, this message ends up spamming quite a bit, since it prints a warning for every frame, which is a tad excessive. As mentioned in this thread, I doubt this line could be beneficial to the user, and needlessly filling the logs might be just worse since it makes it harder to notice warnings in the first place. I don't think it warrants a if_already_warned clause, would it be possible to just drop it? Thanks (please CC)
Hi Vittorio, On Mon, Oct 17, 2016 at 5:28 PM, Vittorio Giovara < vittorio.giovara@gmail.com> wrote: > On Mon, Sep 19, 2016 at 8:36 AM, Ronald S. Bultje <rsbultje@gmail.com> > wrote: > > Hi, > > > > On Fri, Sep 16, 2016 at 9:27 AM, Clément Bœsch <u@pkh.me> wrote: > >> > >> On Fri, Sep 16, 2016 at 03:20:49PM +0200, Vittorio Giovara wrote: > >> > This is the assumption that is made in pixel format conversion do > >> > throughout the code (in particular swscale), and BT-specifications > >> > mandate. > >> > > >> > Add a warning to inform the user that an automatic selection is being > >> > made. > >> > > >> > Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com> > >> > --- > >> > libavfilter/vf_colorspace.c | 9 ++++++--- > >> > 1 file changed, 6 insertions(+), 3 deletions(-) > >> > > >> > diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c > >> > index e69be50..41d1692 100644 > >> > --- a/libavfilter/vf_colorspace.c > >> > +++ b/libavfilter/vf_colorspace.c > >> > @@ -518,10 +518,13 @@ static int convert(AVFilterContext *ctx, void > >> > *data, int job_nr, int n_jobs) > >> > return 0; > >> > } > >> > > >> > -static int get_range_off(int *off, int *y_rng, int *uv_rng, > >> > +static int get_range_off(AVFilterContext *ctx, int *off, > >> > + int *y_rng, int *uv_rng, > >> > enum AVColorRange rng, int depth) > >> > { > >> > switch (rng) { > >> > + case AVCOL_RANGE_UNSPECIFIED: > >> > + av_log(ctx, AV_LOG_WARNING, "Input range not set, assuming > >> > tv/mpeg\n"); > > Hey Ronald, > this message ends up spamming quite a bit, since it prints a warning > for every frame, which is a tad excessive. > > As mentioned in this thread, I doubt this line could be beneficial to > the user, and needlessly filling the logs might be just worse since it > makes it harder to notice warnings in the first place. I don't think > it warrants a if_already_warned clause, would it be possible to just > drop it? If we just print it once and then disable further spamming the console from then onwards, is that OK? Ronald
diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c index e69be50..41d1692 100644 --- a/libavfilter/vf_colorspace.c +++ b/libavfilter/vf_colorspace.c @@ -518,10 +518,13 @@ static int convert(AVFilterContext *ctx, void *data, int job_nr, int n_jobs) return 0; } -static int get_range_off(int *off, int *y_rng, int *uv_rng, +static int get_range_off(AVFilterContext *ctx, int *off, + int *y_rng, int *uv_rng, enum AVColorRange rng, int depth) { switch (rng) { + case AVCOL_RANGE_UNSPECIFIED: + av_log(ctx, AV_LOG_WARNING, "Input range not set, assuming tv/mpeg\n"); case AVCOL_RANGE_MPEG: *off = 16 << (depth - 8); *y_rng = 219 << (depth - 8); @@ -740,7 +743,7 @@ static int create_filtergraph(AVFilterContext *ctx, double rgb2yuv[3][3], (*yuv2rgb)[3] = s->yuv2rgb_dbl_coeffs; int off, bits, in_rng; - res = get_range_off(&off, &s->in_y_rng, &s->in_uv_rng, + res = get_range_off(ctx, &off, &s->in_y_rng, &s->in_uv_rng, s->in_rng, in_desc->comp[0].depth); if (res < 0) { av_log(ctx, AV_LOG_ERROR, @@ -773,7 +776,7 @@ static int create_filtergraph(AVFilterContext *ctx, double (*rgb2yuv)[3] = s->rgb2yuv_dbl_coeffs; int off, out_rng, bits; - res = get_range_off(&off, &s->out_y_rng, &s->out_uv_rng, + res = get_range_off(ctx, &off, &s->out_y_rng, &s->out_uv_rng, s->out_rng, out_desc->comp[0].depth); if (res < 0) { av_log(ctx, AV_LOG_ERROR,
This is the assumption that is made in pixel format conversion do throughout the code (in particular swscale), and BT-specifications mandate. Add a warning to inform the user that an automatic selection is being made. Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com> --- libavfilter/vf_colorspace.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)