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

[Andreas Rheinhardt]

Gautam Ramakrishnan:
> On Wed, Aug 19, 2020 at 5:51 PM <gautamramk at gmail.com> wrote:
>>
>> From: Gautam Ramakrishnan <gautamramk at gmail.com>
>>
>> This patch allows setting a compression ratio and to
>> set multiple layers. The user has to input a compression
>> ratio for each layer.
>> The per layer compression ration can be set as follows:
>> -layer_rates "r1,r2,...rn"
>> for to create 'n' layers.
>> ---
>>  libavcodec/j2kenc.c   | 443 ++++++++++++++++++++++++++++++++++--------
>>  libavcodec/jpeg2000.c |  13 +-
>>  libavcodec/jpeg2000.h |  10 +
>>  3 files changed, 384 insertions(+), 82 deletions(-)
>>

[...]

>> +static int inline check_number(char* st, int* ret) {
>> +    int stlen = strlen(st);
>> +    int i;
>> +    *ret = 0;
>> +    if (stlen <= 0) {
>> +        return AVERROR_INVALIDDATA;
>> +    }
>> +    for (i = 0; i < stlen; i++) {
>> +        if (st[i] >= '0' && st[i] <= '9') {
>> +            *ret = (*ret) * 10 + (st[i] - '0');
>> +        } else {
>> +            return AVERROR_INVALIDDATA;
>> +        }
>> +    }
>> +    return 0;
>> +}
>> +
>> +static int parse_layer_rates(Jpeg2000EncoderContext *s)
>> +{
>> +    int i;
>> +    char* token;
>> +    int rate;
>> +    int nlayers = 0;
>> +    if (!s->lr_str) {
>> +        s->nlayers = 1;
>> +        s->layer_rates[0] = 0;
>> +        return 0;
>> +    }
>> +
>> +    token = strtok(s->lr_str, ",");
>> +    if (!check_number(token, &rate)) {
>> +            s->layer_rates[0] = rate <= 1 ? 0:rate;
>> +            nlayers++;
>> +    } else {
>> +            return AVERROR_INVALIDDATA;
>> +    }
>> +
>> +    while (1) {
>> +        token = strtok(NULL, ",");
>> +        if (!token)
>> +            break;
>> +        if (!check_number(token, &rate)) {
>> +            s->layer_rates[nlayers] = rate <= 1 ? 0:rate;
>> +            nlayers++;
>> +        } else {
>> +            return AVERROR_INVALIDDATA;
>> +        }
>> +    }
>> +
>> +    for (i = 1; i < nlayers; i++) {
>> +        if (s->layer_rates[i] >= s->layer_rates[i-1]) {
>> +            return AVERROR_INVALIDDATA;
>> +        }
>> +    }
>> +    s->nlayers = nlayers;
>> +    return 0;
>> +}
>> +
>>  static av_cold int j2kenc_init(AVCodecContext *avctx)
>>  {
>>      int i, ret;
>> @@ -1388,6 +1664,11 @@ static av_cold int j2kenc_init(AVCodecContext *avctx)
>>
>>      s->avctx = avctx;
>>      av_log(s->avctx, AV_LOG_DEBUG, "init\n");
>> +    if (parse_layer_rates(s)) {
>> +        av_log(s, AV_LOG_WARNING, "Layer rates invalid. Shall encode with 1 layer.\n");
>> +        s->nlayers = 1;
>> +        s->layer_rates[0] = 0;
>> +    }
>>
>>  #if FF_API_PRIVATE_OPT
>>  FF_DISABLE_DEPRECATION_WARNINGS
>> @@ -1408,6 +1689,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
>>      memset(codsty->log2_prec_heights, 15, sizeof(codsty->log2_prec_heights));
>>      codsty->nreslevels2decode=
>>      codsty->nreslevels       = 7;
>> +    codsty->nlayers          = s->nlayers;
>>      codsty->log2_cblk_width  = 4;
>>      codsty->log2_cblk_height = 4;
>>      codsty->transform        = s->pred ? FF_DWT53 : FF_DWT97_INT;
>> @@ -1489,6 +1771,7 @@ static const AVOption options[] = {
>>      { "rpcl",          NULL,                OFFSET(prog),          AV_OPT_TYPE_CONST,   { .i64 = JPEG2000_PGOD_RPCL            }, 0,         0,           VE, "prog" },
>>      { "pcrl",          NULL,                OFFSET(prog),          AV_OPT_TYPE_CONST,   { .i64 = JPEG2000_PGOD_PCRL            }, 0,         0,           VE, "prog" },
>>      { "cprl",          NULL,                OFFSET(prog),          AV_OPT_TYPE_CONST,   { .i64 = JPEG2000_PGOD_CPRL            }, 0,         0,           VE, "prog" },
>> +    { "layer_rates",   "Layer Rates",       OFFSET(lr_str),        AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, VE },
>>      { NULL }
>>  };
>>
>>
>> --
>> 2.17.1
>>
> 
> This patch seems to be breaking FATE.
> I believe that the error is because the patch modifies the encoder
> such that the encoded files will be slightly different now.
> How can this be handled?
> 
Look at the created files and see if they are ok (ideally, they should
show an improvement). If not, modify/drop your patch; otherwise update
the reference files by running make fate GEN=1.

Just by looking at https://patchwork.ffmpeg.org/check/15622/ (i.e.
without taking a look at the actual files and without having run fate
with your patch myself) I can say that your patch is not ok:

-8bb707e596f97451fd325dec2dd610a7 *tests/data/fate/vsynth1-jpeg2000-97.avi
-3654620 tests/data/fate/vsynth1-jpeg2000-97.avi
-5073771a78e1f5366a7eb0df341662fc
*tests/data/fate/vsynth1-jpeg2000-97.out.rawvideo
-stddev:    4.23 PSNR: 35.59 MAXDIFF:   53 bytes:  7603200/  7603200
+5178195043ecfd671d79a194611d3573 *tests/data/fate/vsynth1-jpeg2000-97.avi
+9986568 tests/data/fate/vsynth1-jpeg2000-97.avi
+023623c97973489ba9cf1618b3cd25d3
*tests/data/fate/vsynth1-jpeg2000-97.out.rawvideo
+stddev:    3.58 PSNR: 37.04 MAXDIFF:   49 bytes:  7603200/  7603200

The size of the created files increased from 3654620 to 9986568,
although it seems to me that using multiple layers is supposed to be opt-in.

Besides that: You are using an int[100] array to store the layer rates.
Yet I don't see any check in parse_layer_rates() that actually ensures
that there is no write beyond the end of the array. Besides that
check_number stores the return value of strlen in an int (may be
truncating) and does not check in the calculation.

- Andreas