[FFmpeg-devel] [RFC PATCH v6 3/4] libavcodec/j2kenc: Support for multiple layers

[Moritz Barsnick]

On Wed, Aug 26, 2020 at 21:28:17 +0530, gautamramk at gmail.com wrote:

> + at item layer_rates @var{string}
> +By default, compression is done using the quality metric. This option allows for
> +compression using compression ratio. The compression ratio for each level could
> +be specified. The compression ratio of a layer @code{l} species the what ratio of
> +total file size is contained in the first @code{l} layers.

You should mention what the default behavior is.

> +static int inline check_number(char* st, int* ret) {

Are you reinventing strtol() with different error handling?

> +    token = strtok(s->lr_str, ",");

There's also an av_strtok(), I wonder if that's preferred and of help.

> --- a/tests/ref/vsynth/vsynth1-jpeg2000
> +++ b/tests/ref/vsynth/vsynth1-jpeg2000
> @@ -1,4 +1,4 @@
> -d2a06ad916711d29b30977a06335bb76 *tests/data/fate/vsynth1-jpeg2000.avi
> -2265698 tests/data/fate/vsynth1-jpeg2000.avi
> -15a8e49f6fd014193bbafd72f84936c7 *tests/data/fate/vsynth1-jpeg2000.out.rawvideo
> -stddev:    5.36 PSNR: 33.55 MAXDIFF:   61 bytes:  7603200/  7603200
> +dd66b25f2ebc965eae4c29cfacdd960f *tests/data/fate/vsynth1-jpeg2000.avi
> +2274950 tests/data/fate/vsynth1-jpeg2000.avi
> +b7f48a8965f78011c76483277befc6fc *tests/data/fate/vsynth1-jpeg2000.out.rawvideo
> +stddev:    5.35 PSNR: 33.56 MAXDIFF:   59 bytes:  7603200/  7603200

[...]

Is it really intended to change the default behavior when adding new
options? I expected fate to be unchanged, unless you perhaps describe
why a change was made.

In either case, whether you add options, change default behavior, or
both, it's recommended to bump the library's micro version.

Thanks,
Moritz