[FFmpeg-devel] [RFC PATCH 4/4] libavcodec/j2kenc: Support for multiple layers
Gautam Ramakrishnan
gautamramk at gmail.com
Thu Aug 20 18:16:00 EEST 2020
On Thu, Aug 20, 2020 at 8:25 PM Andreas Rheinhardt
<andreas.rheinhardt at gmail.com> wrote:
>
> Gautam Ramakrishnan:
> > On Thu, Aug 20, 2020 at 12:55 AM Andreas Rheinhardt
> > <andreas.rheinhardt at gmail.com> wrote:
> >>
> >> 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
> >> _______________________________________________
> >> ffmpeg-devel mailing list
> >> ffmpeg-devel at ffmpeg.org
> >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>
> >> To unsubscribe, visit link above, or email
> >> ffmpeg-devel-request at 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.
>
Got it.
> > 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.
Got it. From my understanding, Jpeg2000 compression could be done on
quality metric or compression ratio. So it seems right to preserve the quality
metric based compression and add the use of compression ratio as a new
feature instead of replacing the current functionality.
Thanks a lot for the comments!
>
> - Andreas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
--
-------------
Gautam |
More information about the ffmpeg-devel
mailing list