[FFmpeg-devel,V2,2/3] lavfi/procamp_vaapi: fix the green video issue if without arguments.

From 1d3b388c313881e931928dc4049896265919725d Mon Sep 17 00:00:00 2001
From: Jun Zhao <jun.zhao@intel.com>
Date: Wed, 24 Jan 2018 09:28:24 +0800
Subject: [PATCH V2 2/3] lavfi/procamp_vaapi: fix the green video issue if
 without arguments.

Fix the green output issue when use procamp_vaapi without any
arguments, now if use procamp_vaapi without any arguments, will use
the default value to setting procamp_vaapi.

Signed-off-by: Jun Zhao <jun.zhao@intel.com>
---
 libavfilter/vf_procamp_vaapi.c | 73 ++++++++++++++++++------------------------
 1 file changed, 31 insertions(+), 42 deletions(-)

2018-01-24 4:04 GMT+01:00 Jun Zhao <mypopydev@gmail.com>:

> -        procamp_params[i].type   = VAProcFilterColorBalance;
> -        procamp_params[i].attrib = VAProcColorBalanceBrightness;
> -        procamp_params[i].value  = map(ctx->bright, BRIGHTNESS_MIN, BRIGHTNESS_MAX,
> -                                       procamp_caps[VAProcColorBalanceBrightness-1].range.min_value,
> -                                       procamp_caps[VAProcColorBalanceBrightness-1].range.max_value);
> -        i++;

> -        procamp_params[i].type   = VAProcFilterColorBalance;
> -        procamp_params[i].attrib = VAProcColorBalanceContrast;
> -        procamp_params[i].value  = map(ctx->contrast, CONTRAST_MIN, CONTRAST_MAX,
> -                                       procamp_caps[VAProcColorBalanceContrast-1].range.min_value,
> -                                       procamp_caps[VAProcColorBalanceContrast-1].range.max_value);
> -        i++;

> -        procamp_params[i].type   = VAProcFilterColorBalance;
> -        procamp_params[i].attrib = VAProcColorBalanceHue;
> -        procamp_params[i].value  = map(ctx->hue, HUE_MIN, HUE_MAX,
> -                                       procamp_caps[VAProcColorBalanceHue-1].range.min_value,
> -                                       procamp_caps[VAProcColorBalanceHue-1].range.max_value);
> -        i++;

> -        procamp_params[i].type   = VAProcFilterColorBalance;
> -        procamp_params[i].attrib = VAProcColorBalanceSaturation;
> -        procamp_params[i].value  = map(ctx->saturation, SATURATION_MIN, SATURATION_MAX,
> -                                       procamp_caps[VAProcColorBalanceSaturation-1].range.min_value,
> -                                       procamp_caps[VAProcColorBalanceSaturation-1].range.max_value);
> -        i++;

Please do not reindent these lines in the same patch that removes the
conditions, this has two advantages: It makes reviewing your changes
easier now, and allows somebody who looks at your change in the
future to understand it more quickly.
Send an independent patch for the re-indentation.

Carl Eugen

On 24/01/18 03:04, Jun Zhao wrote:
> 
> From 1d3b388c313881e931928dc4049896265919725d Mon Sep 17 00:00:00 2001
> From: Jun Zhao <jun.zhao@intel.com>
> Date: Wed, 24 Jan 2018 09:28:24 +0800
> Subject: [PATCH V2 2/3] lavfi/procamp_vaapi: fix the green video issue if
>  without arguments.
> 
> Fix the green output issue when use procamp_vaapi without any
> arguments, now if use procamp_vaapi without any arguments, will use
> the default value to setting procamp_vaapi.
> 
> Signed-off-by: Jun Zhao <jun.zhao@intel.com>
> ---
>  libavfilter/vf_procamp_vaapi.c | 73 ++++++++++++++++++------------------------
>  1 file changed, 31 insertions(+), 42 deletions(-)
> 
On 24/01/18 03:04, Jun Zhao wrote:
> 
> From 7a4a53fcc5a9b31b11818976dc02ea8e004d2c5a Mon Sep 17 00:00:00 2001
> From: Jun Zhao <jun.zhao@intel.com>
> Date: Wed, 24 Jan 2018 09:32:50 +0800
> Subject: [PATCH V2 3/3] lavfi/misc_vaapi: use default value setting if without
>  arguments.
> 
> Signed-off-by: Jun Zhao <jun.zhao@intel.com>
> ---
>  libavfilter/vf_misc_vaapi.c | 64 +++++++++++++++++++++------------------------
>  1 file changed, 30 insertions(+), 34 deletions(-)
> 

Ah right, I didn't consider the same issue on the misc filters as well.  Always passing the arguments ends up being a bit simpler too, which is nice.

Both LGTM, tested, applied.

Thanks,

- Mark

2018-01-26 0:21 GMT+01:00 Mark Thompson <sw@jkqxz.net>:
> On 24/01/18 03:04, Jun Zhao wrote:
>>
>> From 1d3b388c313881e931928dc4049896265919725d Mon Sep 17 00:00:00 2001
>> From: Jun Zhao <jun.zhao@intel.com>
>> Date: Wed, 24 Jan 2018 09:28:24 +0800
>> Subject: [PATCH V2 2/3] lavfi/procamp_vaapi: fix the green video issue if
>>  without arguments.
>>
>> Fix the green output issue when use procamp_vaapi without any
>> arguments, now if use procamp_vaapi without any arguments, will use
>> the default value to setting procamp_vaapi.
>>
>> Signed-off-by: Jun Zhao <jun.zhao@intel.com>
>> ---
>>  libavfilter/vf_procamp_vaapi.c | 73 ++++++++++++++++++------------------------
>>  1 file changed, 31 insertions(+), 42 deletions(-)
>>
> On 24/01/18 03:04, Jun Zhao wrote:
>>
>> From 7a4a53fcc5a9b31b11818976dc02ea8e004d2c5a Mon Sep 17 00:00:00 2001
>> From: Jun Zhao <jun.zhao@intel.com>
>> Date: Wed, 24 Jan 2018 09:32:50 +0800
>> Subject: [PATCH V2 3/3] lavfi/misc_vaapi: use default value setting if without
>>  arguments.
>>
>> Signed-off-by: Jun Zhao <jun.zhao@intel.com>
>> ---
>>  libavfilter/vf_misc_vaapi.c | 64 +++++++++++++++++++++------------------------
>>  1 file changed, 30 insertions(+), 34 deletions(-)
>>
>
> Ah right, I didn't consider the same issue on the misc filters as well.  Always passing the arguments ends up being a bit simpler too, which is nice.
>
> Both LGTM, tested, applied.

So you prefer that nobody else comments on patches?

Carl Eugen

On 24/01/18 10:30, Carl Eugen Hoyos wrote:
> 2018-01-24 4:04 GMT+01:00 Jun Zhao <mypopydev@gmail.com>:
> 
>> -        procamp_params[i].type   = VAProcFilterColorBalance;
>> -        procamp_params[i].attrib = VAProcColorBalanceBrightness;
>> -        procamp_params[i].value  = map(ctx->bright, BRIGHTNESS_MIN, BRIGHTNESS_MAX,
>> -                                       procamp_caps[VAProcColorBalanceBrightness-1].range.min_value,
>> -                                       procamp_caps[VAProcColorBalanceBrightness-1].range.max_value);
>> -        i++;
> 
>> -        procamp_params[i].type   = VAProcFilterColorBalance;
>> -        procamp_params[i].attrib = VAProcColorBalanceContrast;
>> -        procamp_params[i].value  = map(ctx->contrast, CONTRAST_MIN, CONTRAST_MAX,
>> -                                       procamp_caps[VAProcColorBalanceContrast-1].range.min_value,
>> -                                       procamp_caps[VAProcColorBalanceContrast-1].range.max_value);
>> -        i++;
> 
>> -        procamp_params[i].type   = VAProcFilterColorBalance;
>> -        procamp_params[i].attrib = VAProcColorBalanceHue;
>> -        procamp_params[i].value  = map(ctx->hue, HUE_MIN, HUE_MAX,
>> -                                       procamp_caps[VAProcColorBalanceHue-1].range.min_value,
>> -                                       procamp_caps[VAProcColorBalanceHue-1].range.max_value);
>> -        i++;
> 
>> -        procamp_params[i].type   = VAProcFilterColorBalance;
>> -        procamp_params[i].attrib = VAProcColorBalanceSaturation;
>> -        procamp_params[i].value  = map(ctx->saturation, SATURATION_MIN, SATURATION_MAX,
>> -                                       procamp_caps[VAProcColorBalanceSaturation-1].range.min_value,
>> -                                       procamp_caps[VAProcColorBalanceSaturation-1].range.max_value);
>> -        i++;
> 
> Please do not reindent these lines in the same patch that removes the
> conditions, this has two advantages: It makes reviewing your changes
> easier now, and allows somebody who looks at your change in the
> future to understand it more quickly.
> Send an independent patch for the re-indentation.

I don't believe this is merited.  I find the change very easy to review now, and in future it will be clearer for these lines to be attributed to the patch which actually modifies them rather than an "indent after previous commit" change.  Such splitting is reasonable where complex blocks change indentation levels while staying mostly or completely the same, but these short blocks do not meet such a threshold.

- Mark