On Sat, Apr 9, 2022 at 5:38 PM James Almer <jamr...@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
_______________________________________________
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".
