diff mbox

[FFmpeg-devel] vf_colorspace: don't spam console with warnings if range is unspecified.

Message ID 1476979574-53380-1-git-send-email-rsbultje@gmail.com
State Accepted
Headers show

Commit Message

Ronald S. Bultje Oct. 20, 2016, 4:06 p.m. UTC 
---
 libavfilter/vf_colorspace.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Josh Dekker Oct. 20, 2016, 5:08 p.m. UTC | #1 
On 20/10/2016 17:06, Ronald S. Bultje wrote:
> ---
>  libavfilter/vf_colorspace.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c
> index c74fe00..f64163f 100644
> --- a/libavfilter/vf_colorspace.c
> +++ b/libavfilter/vf_colorspace.c
> @@ -163,6 +163,8 @@ typedef struct ColorSpaceContext {
>      yuv2yuv_fn yuv2yuv;
>      double yuv2rgb_dbl_coeffs[3][3], rgb2yuv_dbl_coeffs[3][3];
>      int in_y_rng, in_uv_rng, out_y_rng, out_uv_rng;
> +
> +    int did_range_warn;
>  } ColorSpaceContext;
>
>  // FIXME deal with odd width/heights (or just forbid it)
> @@ -523,9 +525,15 @@ static int get_range_off(AVFilterContext *ctx, int *off,
>                           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_UNSPECIFIED: {
> +        ColorSpaceContext *s = ctx->priv;
> +
> +        if (!s->did_range_warn) {
> +            av_log(ctx, AV_LOG_WARNING, "Input range not set, assuming tv/mpeg\n");
> +            s->did_range_warn = 1;
> +        }
>          // fall-through
> +    }
>      case AVCOL_RANGE_MPEG:
>          *off = 16 << (depth - 8);
>          *y_rng = 219 << (depth - 8);
>

I think did_warn_range would sound more natural. LGTM otherwise (with or 
without the change).
Ronald S. Bultje Oct. 20, 2016, 5:11 p.m. UTC | #2 
Hi,

On Thu, Oct 20, 2016 at 1:08 PM, Josh de Kock <josh@itanimul.li> wrote:

> On 20/10/2016 17:06, Ronald S. Bultje wrote:
>
>> ---
>>  libavfilter/vf_colorspace.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c
>> index c74fe00..f64163f 100644
>> --- a/libavfilter/vf_colorspace.c
>> +++ b/libavfilter/vf_colorspace.c
>> @@ -163,6 +163,8 @@ typedef struct ColorSpaceContext {
>>      yuv2yuv_fn yuv2yuv;
>>      double yuv2rgb_dbl_coeffs[3][3], rgb2yuv_dbl_coeffs[3][3];
>>      int in_y_rng, in_uv_rng, out_y_rng, out_uv_rng;
>> +
>> +    int did_range_warn;
>>  } ColorSpaceContext;
>>
>>  // FIXME deal with odd width/heights (or just forbid it)
>> @@ -523,9 +525,15 @@ static int get_range_off(AVFilterContext *ctx, int
>> *off,
>>                           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_UNSPECIFIED: {
>> +        ColorSpaceContext *s = ctx->priv;
>> +
>> +        if (!s->did_range_warn) {
>> +            av_log(ctx, AV_LOG_WARNING, "Input range not set, assuming
>> tv/mpeg\n");
>> +            s->did_range_warn = 1;
>> +        }
>>          // fall-through
>> +    }
>>      case AVCOL_RANGE_MPEG:
>>          *off = 16 << (depth - 8);
>>          *y_rng = 219 << (depth - 8);
>>
>>
> I think did_warn_range would sound more natural. LGTM otherwise (with or
> without the change).


Changed locally, thanks. I'll leave it out for a couple more hours before
pushing so others have time to comment too.

Ronald
Moritz Barsnick Oct. 21, 2016, 8:36 p.m. UTC | #3 
On Thu, Oct 20, 2016 at 12:06:14 -0400, Ronald S. Bultje wrote:
> +            s->did_range_warn = 1;
> +        }
>          // fall-through
> +    }
>      case AVCOL_RANGE_MPEG:

The fall-through comment seems misplaced now (both logically and
probably for Coverity).

Moritz
Ronald S. Bultje Oct. 22, 2016, 12:43 p.m. UTC | #4 
Hi,

On Fri, Oct 21, 2016 at 4:36 PM, Moritz Barsnick <barsnick@gmx.net> wrote:

> On Thu, Oct 20, 2016 at 12:06:14 -0400, Ronald S. Bultje wrote:
> > +            s->did_range_warn = 1;
> > +        }
> >          // fall-through
> > +    }
> >      case AVCOL_RANGE_MPEG:
>
> The fall-through comment seems misplaced now (both logically and
> probably for Coverity).


Is there documentation on what coverity expects?

Ronald
Moritz Barsnick Oct. 22, 2016, 4:19 p.m. UTC | #5 
On Sat, Oct 22, 2016 at 08:43:26 -0400, Ronald S. Bultje wrote:
> Is there documentation on what coverity expects?

I found this:
https://lost-contact.mit.edu/afs/cs.stanford.edu/pkg/prevent-4.3.1/i386_linux26/opt/prevent-linux-4.3.1/cgi-bin/doc/checker_ref.html#c_checker_MISSING_BREAK

Moritz
Ronald S. Bultje Oct. 23, 2016, 2:02 a.m. UTC | #6 
Hi,

On Sat, Oct 22, 2016 at 12:19 PM, Moritz Barsnick <barsnick@gmx.net> wrote:

> On Sat, Oct 22, 2016 at 08:43:26 -0400, Ronald S. Bultje wrote:
> > Is there documentation on what coverity expects?
>
> I found this:
> https://lost-contact.mit.edu/afs/cs.stanford.edu/pkg/
> prevent-4.3.1/i386_linux26/opt/prevent-linux-4.3.1/cgi-
> bin/doc/checker_ref.html#c_checker_MISSING_BREAK


I was hoping for documentation on what it expects, not what can be changed
about it :) Anyway, I'll change it, I don't really care.

Ronald
Ronald S. Bultje Oct. 24, 2016, 8:08 p.m. UTC | #7 
Hi,

On Sat, Oct 22, 2016 at 10:02 PM, Ronald S. Bultje <rsbultje@gmail.com>
wrote:

> Hi,
>
> On Sat, Oct 22, 2016 at 12:19 PM, Moritz Barsnick <barsnick@gmx.net>
> wrote:
>
>> On Sat, Oct 22, 2016 at 08:43:26 -0400, Ronald S. Bultje wrote:
>> > Is there documentation on what coverity expects?
>>
>> I found this:
>> https://lost-contact.mit.edu/afs/cs.stanford.edu/pkg/prevent
>> -4.3.1/i386_linux26/opt/prevent-linux-4.3.1/cgi-bin/
>> doc/checker_ref.html#c_checker_MISSING_BREAK
>
>
> I was hoping for documentation on what it expects, not what can be changed
> about it :) Anyway, I'll change it, I don't really care.
>

Pushed with that modification.

Ronald
Moritz Barsnick Oct. 25, 2016, 9:24 p.m. UTC | #8 
On Sat, Oct 22, 2016 at 22:02:15 -0400, Ronald S. Bultje wrote:
> I was hoping for documentation on what it expects, not what can be changed
> about it :) Anyway, I'll change it, I don't really care.

I had found this to be quite clear:
> End with a comment. The checker assumes that this comment is
> acknowledging a fallthrough. The comment can start anywhere on the
> last line, or be a multi-line C comment.

So: It expects a comment (*any* comment) on the last line (which, to
me, is the exact line preceding the next case label).

Sorry for the late remark,
Moritz
diff mbox

Patch

diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c
index c74fe00..f64163f 100644
--- a/libavfilter/vf_colorspace.c
+++ b/libavfilter/vf_colorspace.c
@@ -163,6 +163,8 @@  typedef struct ColorSpaceContext {
     yuv2yuv_fn yuv2yuv;
     double yuv2rgb_dbl_coeffs[3][3], rgb2yuv_dbl_coeffs[3][3];
     int in_y_rng, in_uv_rng, out_y_rng, out_uv_rng;
+
+    int did_range_warn;
 } ColorSpaceContext;
 
 // FIXME deal with odd width/heights (or just forbid it)
@@ -523,9 +525,15 @@  static int get_range_off(AVFilterContext *ctx, int *off,
                          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_UNSPECIFIED: {
+        ColorSpaceContext *s = ctx->priv;
+
+        if (!s->did_range_warn) {
+            av_log(ctx, AV_LOG_WARNING, "Input range not set, assuming tv/mpeg\n");
+            s->did_range_warn = 1;
+        }
         // fall-through
+    }
     case AVCOL_RANGE_MPEG:
         *off = 16 << (depth - 8);
         *y_rng = 219 << (depth - 8);