[FFmpeg-devel] [PATCH v13 3/4] avcodec/libjxl: add Jpeg XL encoding via libjxl

[Hendrik Leppkes]

On Sat, Apr 9, 2022 at 5:38 PM James Almer <jamrial at gmail.com> wrote:
>
>
>
> On 4/9/2022 10:17 AM, Anton Khirnov wrote:
> > Quoting Leo Izen (2022-04-05 18:55:03)
> >> +static int libjxl_init_jxl_encoder(AVCodecContext *avctx)
> >> +{
> >> +    LibJxlEncodeContext *ctx = avctx->priv_data;
> >> +
> >> +    /* reset the encoder every frame for image2 muxer */
> >> +    JxlEncoderReset(ctx->encoder);
> >> +
> >> +    ctx->options = JxlEncoderFrameSettingsCreate(ctx->encoder, NULL);
> >> +    if (!ctx->options) {
> >> +        av_log(avctx, AV_LOG_ERROR, "Failed to create JxlEncoderOptions\n");
> >> +        return AVERROR_EXTERNAL;
> >> +    }
> >> +
> >> +    /* This needs to be set each time the decoder is reset */
> >> +    if (JxlEncoderSetParallelRunner(ctx->encoder, JxlThreadParallelRunner, ctx->runner)
> >> +            != JXL_ENC_SUCCESS) {
> >> +        av_log(avctx, AV_LOG_ERROR, "Failed to set JxlThreadParallelRunner\n");
> >> +        return AVERROR_EXTERNAL;
> >> +    }
> >> +
> >> +    /* these shouldn't fail, libjxl bug notwithstanding */
> >> +    if (JxlEncoderFrameSettingsSetOption(ctx->options, JXL_ENC_FRAME_SETTING_EFFORT, ctx->effort)
> >> +            != JXL_ENC_SUCCESS) {
> >> +        av_log(avctx, AV_LOG_ERROR, "Failed to set effort to: %d\n", ctx->effort);
> >> +        return AVERROR_EXTERNAL;
> >> +    }
> >> +
> >> +    /* check for negative zero, our default */
> >> +    if (1.0f / ctx->distance == 1.0f / -0.0f) {
> >
> > IIRC division by zero is UB. Why not make the default -1.0 and then just
> > check whether the number is negative?
> >
> >> +    /*
> >> +     * This buffer will be copied when the generic
> >> +     * code makes this packet refcounted,
> >> +     * so we can use the buffer again.
> >> +     */
> >> +    pkt->data = ctx->buffer;
> >> +    pkt->size = bytes_written;
> >
> > This is very evil. Encoders should return refcounted packets and not
> > rely on the generic code fixing stuff up for them.
>
> Agree. Call avcodec_default_get_encode_buffer() then memcpy the data to
> the returned buffer.
> Don't use ff_alloc_packet() as that function does basically the same
> thing you're doing here with a growable non refcounted buffer.
>
> >
> > Also, pointers from av_malloc() cannot be passed to av_realloc(). You
> > need to allocate it with av_realloc() in the first place.
>
> Is this documented? afaik realloc() can be used with malloc'd pointers.
> It will, i assume, also realloc it the first time you call it even if
> you request the exact same amount of memory malloc already allocated.
> But in any case it's hardly a problem if he can just use av_realloc the
> first time.

realloc can work with malloc, but thats not guaranteed with every form
of aligned memory allocator.

That said, I don't think any platform where this is currently a
problem is known, and we likely have this issue in a few areas of the
code.
The documentation of this limitation was removed in 2016
(21f70940ae106bfffa07e73057cdb4b5e81a767a)

- Hendrik