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

[James Almer]

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.