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.
Should I preserve the old functionality if the user does not provide a
"layer_rates"
option?
--
-------------
Gautam |
_______________________________________________
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".
