Gautam Ramakrishnan:
> On Thu, Aug 20, 2020 at 12:55 AM Andreas Rheinhardt
> <andreas.rheinha...@gmail.com> wrote:
>>
>> Gautam Ramakrishnan:
>>> On Wed, Aug 19, 2020 at 5:51 PM <gautamr...@gmail.com> wrote:
>>>>
>>>> From: Gautam Ramakrishnan <gautamr...@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
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
>
> Thanks a lot Andreas. Originally, the encoder used to remove some data
> according to a metric called picture quality.
> This picture quality is a member of avframe.
> However, right now, I have removed that usage and am compressing only
> based on the compression rates provided by the user.
This is confusing: If I am not mistaken, then the encoder did not only
use to remove some data based upon the picture quality metric; it (i.e.
git master) still does and it is this very patch that intends to change
this. Typically we use the present tense to talk about current git master.
> Should I preserve the old functionality if the user does not provide a
> "layer_rates"
> option?
>
If your patch breaks the current way of signalling the desired quality
and leads to a massive increase in filesize for people using the current
way of signalling the quality, then you will be breaking lots of current
users' usecases. I don't think that's acceptable.
- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
