Re: [FFmpeg-devel] [PATCHv2] vf_colorspace: Interpret unspecified color range as limited range
Hi Vittorio,
On Mon, Oct 17, 2016 at 5:28 PM, Vittorio Giovara <
vittorio.giov...@gmail.com> wrote:
> On Mon, Sep 19, 2016 at 8:36 AM, Ronald S. Bultje
> wrote:
> > Hi,
> >
> > On Fri, Sep 16, 2016 at 9:27 AM, Clément Bœsch 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
> >> > ---
> >> > 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCHv2] vf_colorspace: Interpret unspecified color range as limited range
On Mon, Sep 19, 2016 at 8:36 AM, Ronald S. Bultje wrote:
> Hi,
>
> On Fri, Sep 16, 2016 at 9:27 AM, Clément Bœsch 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
>> > ---
>> > 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)
--
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCHv2] vf_colorspace: Interpret unspecified color range as limited range
Hi,
On Fri, Sep 16, 2016 at 9:27 AM, Clément Bœsch 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
> > ---
> > 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCHv2] vf_colorspace: Interpret unspecified color range as limited range
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
> ---
> 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.
[...]
--
Clément B.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCHv2] vf_colorspace: Interpret unspecified color range as limited range
Hi,
On Fri, Sep 16, 2016 at 9:20 AM, Vittorio Giovara <
vittorio.giov...@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
> ---
> 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
