[FFmpeg-devel] Select cubic and lanczos as alternative where super-sampling is not supported

On 09/09/16 11:03, Carl Eugen Hoyos wrote:
> 2016-09-09 11:55 GMT+02:00 Sven C. Dack <sven.c.dack@sky.com>:
>
>> Super-sampling is currently only supported by CUDA/NPP when the output
>> dimensions are both smaller than the input dimensions. The patch lets ffmpeg
>> select an alternative algorithm and prints a warning in such cases.
> Tabs (as in your patch) cannot be committed to our git repository, please
> remove them.
> You can use the script tools/patcheck to find them.
>

Ok, thank you.

It tells me my change log entry is missing.

I've commited it with: git commit vf_scale_npp.c

Entered my comment and applied format-patch: git format-patch -1 vf_scale_npp.c

How would I get the missing change log entry in there?

Sven

2016-09-09 12:13 GMT+02:00 Sven C. Dack <sven.c.dack@sky.com>:

> It tells me my change log entry is missing.
>
> I've commited it with: git commit vf_scale_npp.c
>
> Entered my comment and applied format-patch: git format-patch -1
> vf_scale_npp.c
>
> How would I get the missing change log entry in there?

The change to the file Changelog in FFmpeg's root is only
necessary if you make a major addition to FFmpeg (like a
new filter, not a fix for an existing filter).

Carl Eugen

Moin Sven,

On Fri, Sep 09, 2016 at 11:13:55 +0100, Sven C. Dack wrote:

I may be missing something, but my excuse is that I can't test, but just
inspect by looking at it:

> +       if (s->interp_algo == NPPI_INTER_SUPER &&
> +           (out_width > in_width && out_height > in_height)) {
> +           s->interp_algo = NPPI_INTER_LANCZOS;
> +           av_log(ctx, AV_LOG_WARNING, "super-sampling not supported for output dimensions, using lanczos instead.\n");
> +       }
> +       if (s->interp_algo == NPPI_INTER_SUPER &&
> +           !(out_width < in_width && out_height < in_height)) {
> +           s->interp_algo = NPPI_INTER_CUBIC;
> +           av_log(ctx, AV_LOG_WARNING, "super-sampling not supported for output dimensions, using cubic instead.\n");
> +       }
> +    }

Let's assume s->interp_algo=NPPI_INTER_SUPER, out_width>in_width,
out_height>in_height:

if (true && (true && true)) {  }
if (true && !(false && false) {  }

Both blocks will be entered! Didn't you see both messages when testing?

Your commit message says ffmpeg needs to choose a different algo if
both dimensions aren't smaller, that's the second if(), which should be
the top-level decider:

> +       if (s->interp_algo == NPPI_INTER_SUPER &&
> +           !(out_width < in_width && out_height < in_height)) {

Then you seem to choose lanczos only if both are greater, so let me do
this as such:
> +           if (out_width > in_width && out_height > in_height) {
> +               s->interp_algo = NPPI_INTER_LANCZOS;
> +               av_log(ctx, AV_LOG_WARNING, "super-sampling not supported for output dimensions, using lanczos instead.\n");
> +           }
then the rest (i.e. one larger or equal, one smaller) would be cubic:
> +           else {
> +               s->interp_algo = NPPI_INTER_CUBIC;
> +               av_log(ctx, AV_LOG_WARNING, "super-sampling not supported for output dimensions, using cubic instead.\n");
> +           }
> +       }

Was that the intent?

Gruß,
Moritz

On 11/09/16 18:55, Moritz Barsnick wrote:
> Moin Sven,
>
> On Fri, Sep 09, 2016 at 11:13:55 +0100, Sven C. Dack wrote:
>
> I may be missing something, but my excuse is that I can't test, but just
> inspect by looking at it:
>
>> +       if (s->interp_algo == NPPI_INTER_SUPER &&
>> +           (out_width > in_width && out_height > in_height)) {
>> +           s->interp_algo = NPPI_INTER_LANCZOS;
>> +           av_log(ctx, AV_LOG_WARNING, "super-sampling not supported for output dimensions, using lanczos instead.\n");
>> +       }
>> +       if (s->interp_algo == NPPI_INTER_SUPER &&
>> +           !(out_width < in_width && out_height < in_height)) {
>> +           s->interp_algo = NPPI_INTER_CUBIC;
>> +           av_log(ctx, AV_LOG_WARNING, "super-sampling not supported for output dimensions, using cubic instead.\n");
>> +       }
>> +    }

The value s->interp_algo gets change in each body. There is no way it can enter 
the second body if it entered the first, because the value is part of both the 
conditions.

I thought about some neat logic at first, but decided to do it this way for more 
readability. Now you've put me on the spot... Your call.

Sven

On Sun, Sep 11, 2016 at 19:04:39 +0100, Sven C. Dack wrote:
> the second body if it entered the first, because the value is part of both the 
> conditions.

D'uh, I missed that part. (It wasn't obvious enough to me. ;-))

> I thought about some neat logic at first, but decided to do it this
> way for more readability. Now you've put me on the spot... Your call.

In that case, the code you wrote is just fine, albeit a bit more
difficult to grasp - for me.

It's not my call at all, I'm just commenting.

Moritz