[FFmpeg-devel,v2,1/1] avcodec/libjxlenc: Add libjxl_animated encoder

---
 configure              |   1 +
 libavcodec/allcodecs.c |   1 +
 libavcodec/libjxlenc.c | 214 +++++++++++++++++++++++++++++++++--------
 3 files changed, 177 insertions(+), 39 deletions(-)

On 12/13/23 15:53, Zsolt Vadász via ffmpeg-devel wrote:
> ---
>   configure              |   1 +
>   libavcodec/allcodecs.c |   1 +
>   libavcodec/libjxlenc.c | 214 +++++++++++++++++++++++++++++++++--------
>   3 files changed, 177 insertions(+), 39 deletions(-)
> 
> diff --git a/configure b/configure
> index 7d2ee66000..a334a9b1e0 100755
> --- a/configure
> +++ b/configure
> @@ -3418,6 +3418,7 @@ libilbc_decoder_deps="libilbc"
>   libilbc_encoder_deps="libilbc"
>   libjxl_decoder_deps="libjxl libjxl_threads"
>   libjxl_encoder_deps="libjxl libjxl_threads"
> +libjxl_animated_encoder_deps="libjxl libjxl_threads"
>   libkvazaar_encoder_deps="libkvazaar"
>   libmodplug_demuxer_deps="libmodplug"
>   libmp3lame_encoder_deps="libmp3lame"
> diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
> index b0f004e15c..e6733b0d4f 100644
> --- a/libavcodec/allcodecs.c
> +++ b/libavcodec/allcodecs.c
> @@ -784,6 +784,7 @@ extern const FFCodec ff_libilbc_encoder;
>   extern const FFCodec ff_libilbc_decoder;
>   extern const FFCodec ff_libjxl_decoder;
>   extern const FFCodec ff_libjxl_encoder;
> +extern const FFCodec ff_libjxl_animated_encoder;

This should be called libjxl_anim for consistency. Compare with 
libwebp_anim and jpegxl_anim demuxer.


>   extern const FFCodec ff_libmp3lame_encoder;
>   extern const FFCodec ff_libopencore_amrnb_encoder;
>   extern const FFCodec ff_libopencore_amrnb_decoder;
> diff --git a/libavcodec/libjxlenc.c b/libavcodec/libjxlenc.c
> index d707f3a61b..bf44307a34 100644
> --- a/libavcodec/libjxlenc.c
> +++ b/libavcodec/libjxlenc.c
> @@ -39,6 +39,7 @@
>   #include "avcodec.h"
>   #include "encode.h"
>   #include "codec_internal.h"
> +#include "internal.h"
>   
>   #include <jxl/encode.h>
>   #include <jxl/thread_parallel_runner.h>
> @@ -49,11 +50,15 @@ typedef struct LibJxlEncodeContext {
>       void *runner;
>       JxlEncoder *encoder;
>       JxlEncoderFrameSettings *options;
> +    JxlPixelFormat jxl_fmt;
>       int effort;
>       float distance;
>       int modular;
>       uint8_t *buffer;
>       size_t buffer_size;
> +    /* Only used by libjxl_animated */
> +    int animated;

It's not actually true that ctx->animated is only used by libjxl_anim 
because it's checked by the standard encoder as well.

> +    AVFrame *last;

I don't see the purpose of keeping this here.

>   } LibJxlEncodeContext;
>   
>   /**
> @@ -183,6 +188,8 @@ static av_cold int libjxl_encode_init(AVCodecContext *avctx)
>           return AVERROR(ENOMEM);
>       }
>   
> +    ctx->animated = 0;
> +
>       return 0;
>   }
>   
> @@ -237,28 +244,19 @@ static int libjxl_populate_primaries(void *avctx, JxlColorEncoding *jxl_color, e
>       return 0;
>   }
>   
> -/**
> - * Encode an entire frame. Currently animation, is not supported by
> - * this encoder, so this will always reinitialize a new still image
> - * and encode a one-frame image (for image2 and image2pipe).
> - */
> -static int libjxl_encode_frame(AVCodecContext *avctx, AVPacket *pkt, const AVFrame *frame, int *got_packet)
> +static int libjxl_encode_init_image(AVCodecContext *avctx, const AVFrame *frame)
>   {
>       LibJxlEncodeContext *ctx = avctx->priv_data;
>       AVFrameSideData *sd;
>       const AVPixFmtDescriptor *pix_desc = av_pix_fmt_desc_get(frame->format);
>       JxlBasicInfo info;
>       JxlColorEncoding jxl_color;
> -    JxlPixelFormat jxl_fmt;
> +    JxlPixelFormat *jxl_fmt = &ctx->jxl_fmt;
>       int bits_per_sample;
>   #if JPEGXL_NUMERIC_VERSION >= JPEGXL_COMPUTE_NUMERIC_VERSION(0, 8, 0)
>       JxlBitDepth jxl_bit_depth;
>   #endif
> -    JxlEncoderStatus jret;
>       int ret;
> -    size_t available = ctx->buffer_size;
> -    size_t bytes_written = 0;
> -    uint8_t *next_out = ctx->buffer;
>   
>       ret = libjxl_init_jxl_encoder(avctx);
>       if (ret) {
> @@ -268,23 +266,30 @@ static int libjxl_encode_frame(AVCodecContext *avctx, AVPacket *pkt, const AVFra
>   
>       /* populate the basic info settings */
>       JxlEncoderInitBasicInfo(&info);
> -    jxl_fmt.num_channels = pix_desc->nb_components;
> +    jxl_fmt->num_channels = pix_desc->nb_components;
>       info.xsize = frame->width;
>       info.ysize = frame->height;
> -    info.num_extra_channels = (jxl_fmt.num_channels + 1) % 2;
> -    info.num_color_channels = jxl_fmt.num_channels - info.num_extra_channels;
> -    bits_per_sample = av_get_bits_per_pixel(pix_desc) / jxl_fmt.num_channels;
> +    info.num_extra_channels = (jxl_fmt->num_channels + 1) % 2;
> +    info.num_color_channels = jxl_fmt->num_channels - info.num_extra_channels;
> +    bits_per_sample = av_get_bits_per_pixel(pix_desc) / jxl_fmt->num_channels;
>       info.bits_per_sample = avctx->bits_per_raw_sample > 0 && !(pix_desc->flags & AV_PIX_FMT_FLAG_FLOAT)
>                              ? avctx->bits_per_raw_sample : bits_per_sample;
>       info.alpha_bits = (info.num_extra_channels > 0) * info.bits_per_sample;
>       if (pix_desc->flags & AV_PIX_FMT_FLAG_FLOAT) {
>           info.exponent_bits_per_sample = info.bits_per_sample > 16 ? 8 : 5;
>           info.alpha_exponent_bits = info.alpha_bits ? info.exponent_bits_per_sample : 0;
> -        jxl_fmt.data_type = info.bits_per_sample > 16 ? JXL_TYPE_FLOAT : JXL_TYPE_FLOAT16;
> +        jxl_fmt->data_type = info.bits_per_sample > 16 ? JXL_TYPE_FLOAT : JXL_TYPE_FLOAT16;
>       } else {
>           info.exponent_bits_per_sample = 0;
>           info.alpha_exponent_bits = 0;
> -        jxl_fmt.data_type = info.bits_per_sample <= 8 ? JXL_TYPE_UINT8 : JXL_TYPE_UINT16;
> +        jxl_fmt->data_type = info.bits_per_sample <= 8 ? JXL_TYPE_UINT8 : JXL_TYPE_UINT16;
> +    }
> +    if(ctx->animated) {
> +        info.have_animation = 1;
> +        info.animation.have_timecodes = 0;
> +        info.animation.num_loops = 0;
> +        info.animation.tps_numerator = frame->time_base.den;
> +        info.animation.tps_denominator = frame->time_base.num;
>       }
>   
>   #if JPEGXL_NUMERIC_VERSION >= JPEGXL_COMPUTE_NUMERIC_VERSION(0, 8, 0)
> @@ -382,19 +387,19 @@ static int libjxl_encode_frame(AVCodecContext *avctx, AVPacket *pkt, const AVFra
>               av_log(avctx, AV_LOG_WARNING, "Could not increase codestream level\n");
>       }
>   
> -    jxl_fmt.endianness = JXL_NATIVE_ENDIAN;
> -    jxl_fmt.align = frame->linesize[0];
> +    jxl_fmt->endianness = JXL_NATIVE_ENDIAN;
> +    jxl_fmt->align = frame->linesize[0];
>   
> -    if (JxlEncoderAddImageFrame(ctx->options, &jxl_fmt, frame->data[0], jxl_fmt.align * info.ysize) != JXL_ENC_SUCCESS) {
> -        av_log(avctx, AV_LOG_ERROR, "Failed to add Image Frame\n");
> -        return AVERROR_EXTERNAL;
> -    }
>   
> -    /*
> -     * Run this after the last frame in the image has been passed.
> -     * TODO support animation
> -     */
> -    JxlEncoderCloseInput(ctx->encoder);
> +    return 0;
> +}
> +
> +static int libjxl_encode_process_output(AVCodecContext *avctx, size_t *bytes_written)
> +{
> +    LibJxlEncodeContext *ctx = avctx->priv_data;
> +    JxlEncoderStatus jret;
> +    size_t available = ctx->buffer_size;
> +    uint8_t *next_out = ctx->buffer;
>   
>       while (1) {
>           jret = JxlEncoderProcessOutput(ctx->encoder, &next_out, &available);
> @@ -402,7 +407,7 @@ static int libjxl_encode_frame(AVCodecContext *avctx, AVPacket *pkt, const AVFra
>               av_log(avctx, AV_LOG_ERROR, "Unspecified libjxl error occurred\n");
>               return AVERROR_EXTERNAL;
>           }
> -        bytes_written = ctx->buffer_size - available;
> +        *bytes_written = ctx->buffer_size - available;
>           /* all data passed has been encoded */
>           if (jret == JXL_ENC_SUCCESS)
>               break;
> @@ -419,14 +424,48 @@ static int libjxl_encode_frame(AVCodecContext *avctx, AVPacket *pkt, const AVFra
>                   return AVERROR(ENOMEM);
>               ctx->buffer = temp;
>               ctx->buffer_size = new_size;
> -            next_out = ctx->buffer + bytes_written;
> -            available = new_size - bytes_written;
> +            next_out = ctx->buffer + *bytes_written;
> +            available = new_size - *bytes_written;
>               continue;
>           }
>           av_log(avctx, AV_LOG_ERROR, "Bad libjxl event: %d\n", jret);
>           return AVERROR_EXTERNAL;
>       }
>   
> +    return 0;
> +}
> +
> +/**
> + * Encode an entire frame. Currently animation, is not supported by
> + * this encoder, so this will always reinitialize a new still image
> + * and encode a one-frame image (for image2 and image2pipe).
> + */

Watch your copy/paste.

> +static int libjxl_encode_frame(AVCodecContext *avctx, AVPacket *pkt, const AVFrame *frame, int *got_packet)
> +{
> +    LibJxlEncodeContext *ctx = avctx->priv_data;
> +    JxlPixelFormat *jxl_fmt = &ctx->jxl_fmt;
> +    int ret;
> +    size_t bytes_written = 0;
> +
> +    if((ret = libjxl_encode_init_image(avctx, frame)) < 0)
> +        return ret;
> +
> +    if (JxlEncoderAddImageFrame(ctx->options,
> +                                jxl_fmt,
> +                                frame->data[0],
> +                                jxl_fmt->align * frame->height) != JXL_ENC_SUCCESS) {
> +        av_log(avctx, AV_LOG_ERROR, "Failed to add Image Frame\n");
> +        return AVERROR_EXTERNAL;
> +    }
> +
> +    /*
> +     * Run this after the last frame in the image has been passed.
> +     */
> +    JxlEncoderCloseInput(ctx->encoder);
> +
> +    if((ret = libjxl_encode_process_output(avctx, &bytes_written)) < 0)
> +        return ret;
> +
>       ret = ff_get_encode_buffer(avctx, pkt, bytes_written, 0);
>       if (ret < 0)
>           return ret;
> @@ -437,6 +476,70 @@ static int libjxl_encode_frame(AVCodecContext *avctx, AVPacket *pkt, const AVFra
>       return 0;
>   }
>   
> +static int libjxl_animated_encode_frame(AVCodecContext *avctx, AVPacket *pkt, const AVFrame *frame, int *got_packet)
> +{
> +    LibJxlEncodeContext *ctx = avctx->priv_data;
> +    int ret = 0;
> +
> +    if(avctx->frame_num == 0)
> +        if((ret = libjxl_encode_init_image(avctx, frame)) < 0)
> +            return ret;

Don't rely on avctx->frame_num to be set. You should track the 
sent-initial-frame status separately.


> +
> +    if(!ctx->last && !avctx->internal->draining) { > +        ctx->last = av_frame_clone(frame);
> +        *got_packet = 0;
> +        return AVERROR(EAGAIN);
It looks like you're trying to emit one packet per image, rather than 
one packet per encoded frame. This is fine, but you should not be 
calling av_frame_clone, and there's no reason to use 
avctx->internal->draining here. If you are doing this, you also have no 
reason to cache ctx->last at all.


> +    } else if(!ctx->last && avctx->internal->draining && !frame) {
> +        *got_packet = 0;
> +    } else {
> +        JxlPixelFormat *jxl_fmt = &ctx->jxl_fmt;
> +        JxlFrameHeader frame_header;
> +        size_t bytes_written = 0;
> +
> +        JxlEncoderInitFrameHeader(&frame_header);
> +        frame_header.duration = ctx->last->duration;

AVFrame->duration may be zero if unknown. You need to track PTS instead.

> +        pkt->duration = ctx->last->duration;
> +        pkt->pts = AV_NOPTS_VALUE;
> +        pkt->dts = AV_NOPTS_VALUE;

Do not do this. Set the PTS and DTS correctly.


> +
> +        if(JxlEncoderSetFrameHeader(ctx->options, &frame_header) != JXL_ENC_SUCCESS) {
> +            av_log(avctx, AV_LOG_ERROR, "Failed to set frame header\n");
> +            return AVERROR_EXTERNAL;
> +        }
> +        if (JxlEncoderAddImageFrame(ctx->options,
> +                                    jxl_fmt,
> +                                    ctx->last->data[0],
> +                                    jxl_fmt->align * ctx->last->height) != JXL_ENC_SUCCESS) {
> +            av_log(avctx, AV_LOG_ERROR, "Failed to add Image Frame\n");
> +            return AVERROR_EXTERNAL;
> +        }
> +
> +        /*
> +         * Run this after the last frame in the image has been passed.
> +         */
> +        if(avctx->internal->draining)
> +            JxlEncoderCloseInput(ctx->encoder);


avctx->internal->draining is not a reliable check. When nothing is left 
to encode, the encoder will receive NULL as the AVFrame argument.

> +
> +        if((ret = libjxl_encode_process_output(avctx, &bytes_written)) < 0)
> +            return ret;
> +
> +        ret = ff_get_encode_buffer(avctx, pkt, bytes_written, 0);
> +        if (ret < 0)
> +            return ret;
> +
> +        memcpy(pkt->data, ctx->buffer, bytes_written);
> +        *got_packet = 1;
> +
> +        if(!avctx->internal->draining) {
> +            av_frame_replace(ctx->last, frame);
> +        } else {
> +            av_frame_free(&ctx->last);
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>   static av_cold int libjxl_encode_close(AVCodecContext *avctx)
>   {
>       LibJxlEncodeContext *ctx = avctx->priv_data;
> @@ -458,6 +561,17 @@ static av_cold int libjxl_encode_close(AVCodecContext *avctx)
>       return 0;
>   }
>   
> +static av_cold int libjxl_animated_encode_init(AVCodecContext *avctx)
> +{
> +    int ret;
> +    LibJxlEncodeContext *ctx = avctx->priv_data;
> +    if((ret = libjxl_encode_init(avctx)) < 0)
> +        return ret;
> +    ctx->animated = 1;
> +    ret = libjxl_init_jxl_encoder(avctx);
> +    return ret;
> +}
> +
>   #define OFFSET(x) offsetof(LibJxlEncodeContext, x)
>   #define VE AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_ENCODING_PARAM
>   
> @@ -476,6 +590,15 @@ static const AVClass libjxl_encode_class = {
>       .version    = LIBAVUTIL_VERSION_INT,
>   };
>   
> +static const enum AVPixelFormat libjxl_pix_fmts[] = {
> +    AV_PIX_FMT_RGB24, AV_PIX_FMT_RGBA,
> +    AV_PIX_FMT_RGB48, AV_PIX_FMT_RGBA64,
> +    AV_PIX_FMT_GRAY8, AV_PIX_FMT_YA8,
> +    AV_PIX_FMT_GRAY16, AV_PIX_FMT_YA16,
> +    AV_PIX_FMT_GRAYF32,
> +    AV_PIX_FMT_NONE
> +};
> +
>   const FFCodec ff_libjxl_encoder = {
>       .p.name           = "libjxl",
>       CODEC_LONG_NAME("libjxl JPEG XL"),
> @@ -490,14 +613,27 @@ const FFCodec ff_libjxl_encoder = {
>       .caps_internal    = FF_CODEC_CAP_NOT_INIT_THREADSAFE |
>                           FF_CODEC_CAP_AUTO_THREADS | FF_CODEC_CAP_INIT_CLEANUP |
>                           FF_CODEC_CAP_ICC_PROFILES,
> -    .p.pix_fmts       = (const enum AVPixelFormat[]) {
> -        AV_PIX_FMT_RGB24, AV_PIX_FMT_RGBA,
> -        AV_PIX_FMT_RGB48, AV_PIX_FMT_RGBA64,
> -        AV_PIX_FMT_GRAY8, AV_PIX_FMT_YA8,
> -        AV_PIX_FMT_GRAY16, AV_PIX_FMT_YA16,
> -        AV_PIX_FMT_GRAYF32,
> -        AV_PIX_FMT_NONE
> -    },
> +    .p.pix_fmts       = libjxl_pix_fmts,
> +    .p.priv_class     = &libjxl_encode_class,
> +    .p.wrapper_name   = "libjxl",
> +};
> +
> +const FFCodec ff_libjxl_animated_encoder = {
> +    .p.name           = "libjxl_animated",
> +    CODEC_LONG_NAME("libjxl Animated JPEG XL"),
> +    .p.type           = AVMEDIA_TYPE_VIDEO,
> +    .p.id             = AV_CODEC_ID_JPEGXL,
> +    .priv_data_size   = sizeof(LibJxlEncodeContext),
> +    .init             = libjxl_animated_encode_init,
> +    FF_CODEC_ENCODE_CB(libjxl_animated_encode_frame),
> +    .close            = libjxl_encode_close,
> +    .p.capabilities   = AV_CODEC_CAP_OTHER_THREADS |
> +                        AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE |
> +                        AV_CODEC_CAP_DELAY,
> +    .caps_internal    = FF_CODEC_CAP_NOT_INIT_THREADSAFE |
> +                        FF_CODEC_CAP_AUTO_THREADS | FF_CODEC_CAP_INIT_CLEANUP |
> +                        FF_CODEC_CAP_ICC_PROFILES | FF_CODEC_CAP_EOF_FLUSH,

Why FF_CODEC_CAP_EOF_FLUSH?


> +    .p.pix_fmts       = libjxl_pix_fmts,
>       .p.priv_class     = &libjxl_encode_class,
>       .p.wrapper_name   = "libjxl",
>   };

On Friday, December 15th, 2023 at 12:20 AM, Leo Izen <leo.izen@gmail.com> wrote:


> > + AVFrame *last;
> 
> 
> I don't see the purpose of keeping this here.
> 

The name is misleading, I should have named it `previous`, since it always contains the previous frame.
I did it this way so I could call JxlEncoderCloseInput when the callback received NULL.
> > +
> > + if(!ctx->last && !avctx->internal->draining) { > + ctx->last = av_frame_clone(frame);
> > + *got_packet = 0;
> > + return AVERROR(EAGAIN);
> 
> It looks like you're trying to emit one packet per image, rather than
> one packet per encoded frame. This is fine, but you should not be
> calling av_frame_clone, and there's no reason to use
> avctx->internal->draining here. If you are doing this, you also have no
> 
> reason to cache ctx->last at all.

It's the opposite, I'm trying to emit a packet for each frame of the animation.
> > +const FFCodec ff_libjxl_animated_encoder = {
> > + .p.name = "libjxl_animated",
> > + CODEC_LONG_NAME("libjxl Animated JPEG XL"),
> > + .p.type = AVMEDIA_TYPE_VIDEO,
> > + .p.id = AV_CODEC_ID_JPEGXL,
> > + .priv_data_size = sizeof(LibJxlEncodeContext),
> > + .init = libjxl_animated_encode_init,
> > + FF_CODEC_ENCODE_CB(libjxl_animated_encode_frame),
> > + .close = libjxl_encode_close,
> > + .p.capabilities = AV_CODEC_CAP_OTHER_THREADS |
> > + AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE |
> > + AV_CODEC_CAP_DELAY,
> > + .caps_internal = FF_CODEC_CAP_NOT_INIT_THREADSAFE |
> > + FF_CODEC_CAP_AUTO_THREADS | FF_CODEC_CAP_INIT_CLEANUP |
> > + FF_CODEC_CAP_ICC_PROFILES | FF_CODEC_CAP_EOF_FLUSH,
> 
> 
> Why FF_CODEC_CAP_EOF_FLUSH?

So the encoder receives a NULL after the last frame has been submitted,
so JxlEncoderCloseInput can be called and the final frame properly emitted.

On 12/15/23 11:40, Zsolt Vadász via ffmpeg-devel wrote:
> On Friday, December 15th, 2023 at 12:20 AM, Leo Izen <leo.izen@gmail.com> wrote:
> 
> 
>>> + AVFrame *last;
>>
>>
>> I don't see the purpose of keeping this here.
>>
> 
> The name is misleading, I should have named it `previous`, since it always contains the previous frame.
> I did it this way so I could call JxlEncoderCloseInput when the callback received NULL.

This isn't needed to call JxlEncoderCloseInput, as it takes one 
argument, which is JxlEncoder *.


>>> +
>>> + if(!ctx->last && !avctx->internal->draining) { > + ctx->last = av_frame_clone(frame);
>>> + *got_packet = 0;
>>> + return AVERROR(EAGAIN);
>>
>> It looks like you're trying to emit one packet per image, rather than
>> one packet per encoded frame. This is fine, but you should not be
>> calling av_frame_clone, and there's no reason to use
>> avctx->internal->draining here. If you are doing this, you also have no
>> reason to cache ctx->last at all.
> 
> It's the opposite, I'm trying to emit a packet for each frame of the animation.

Libjxl provides no promise of doing this meaningfully, by the way. You 
may end up with arbitrary subdivisions, not subdivisions on frame 
boundaries.

>>> +const FFCodec ff_libjxl_animated_encoder = {
>>> + .p.name = "libjxl_animated",
>>> + CODEC_LONG_NAME("libjxl Animated JPEG XL"),
>>> + .p.type = AVMEDIA_TYPE_VIDEO,
>>> + .p.id = AV_CODEC_ID_JPEGXL,
>>> + .priv_data_size = sizeof(LibJxlEncodeContext),
>>> + .init = libjxl_animated_encode_init,
>>> + FF_CODEC_ENCODE_CB(libjxl_animated_encode_frame),
>>> + .close = libjxl_encode_close,
>>> + .p.capabilities = AV_CODEC_CAP_OTHER_THREADS |
>>> + AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE |
>>> + AV_CODEC_CAP_DELAY,
>>> + .caps_internal = FF_CODEC_CAP_NOT_INIT_THREADSAFE |
>>> + FF_CODEC_CAP_AUTO_THREADS | FF_CODEC_CAP_INIT_CLEANUP |
>>> + FF_CODEC_CAP_ICC_PROFILES | FF_CODEC_CAP_EOF_FLUSH,
>>
>>
>> Why FF_CODEC_CAP_EOF_FLUSH?
> 
> So the encoder receives a NULL after the last frame has been submitted,
> so JxlEncoderCloseInput can be called and the final frame properly emitted.

Ah, that makes sense, thanks.

On Friday, December 15th, 2023 at 10:12 PM, Leo Izen <leo.izen@gmail.com> wrote:


> On 12/15/23 11:40, Zsolt Vadász via ffmpeg-devel wrote:
> 
> > On Friday, December 15th, 2023 at 12:20 AM, Leo Izen leo.izen@gmail.com wrote:
> > 
> > > > + AVFrame *last;
> > > 
> > > I don't see the purpose of keeping this here.
> > 
> > The name is misleading, I should have named it `previous`, since it always contains the previous frame.
> > I did it this way so I could call JxlEncoderCloseInput when the callback received NULL.
> 
> 
> This isn't needed to call JxlEncoderCloseInput, as it takes one
> argument, which is JxlEncoder *.

Indeed, but according to the docs[0]:
"If the last frame or last box has been added, JxlEncoderCloseInput, JxlEncoderCloseFrames
and/or JxlEncoderCloseBoxes must be called before the next JxlEncoderProcessOutput call,
or the codestream won’t be encoded correctly."
When the encoder eventually receives NULL, there isn't anything left to add, unless I store the previous frame,
is there?
> > > > +
> > > > + if(!ctx->last && !avctx->internal->draining) { > + ctx->last = av_frame_clone(frame);
> > > > + *got_packet = 0;
> > > > + return AVERROR(EAGAIN);
> > > 
> > > It looks like you're trying to emit one packet per image, rather than
> > > one packet per encoded frame. This is fine, but you should not be
> > > calling av_frame_clone, and there's no reason to use
> > > avctx->internal->draining here. If you are doing this, you also have no
> > > reason to cache ctx->last at all.
> > 
> > It's the opposite, I'm trying to emit a packet for each frame of the animation.
> 
> 
> Libjxl provides no promise of doing this meaningfully, by the way. You
> may end up with arbitrary subdivisions, not subdivisions on frame
> boundaries.
> 
Well that's one thing I didn't account for, a real pity. Do you recommend emitting a
single packet instead?
> > > > +const FFCodec ff_libjxl_animated_encoder = {
> > > > + .p.name = "libjxl_animated",
> > > > + CODEC_LONG_NAME("libjxl Animated JPEG XL"),
> > > > + .p.type = AVMEDIA_TYPE_VIDEO,
> > > > + .p.id = AV_CODEC_ID_JPEGXL,
> > > > + .priv_data_size = sizeof(LibJxlEncodeContext),
> > > > + .init = libjxl_animated_encode_init,
> > > > + FF_CODEC_ENCODE_CB(libjxl_animated_encode_frame),
> > > > + .close = libjxl_encode_close,
> > > > + .p.capabilities = AV_CODEC_CAP_OTHER_THREADS |
> > > > + AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE |
> > > > + AV_CODEC_CAP_DELAY,
> > > > + .caps_internal = FF_CODEC_CAP_NOT_INIT_THREADSAFE |
> > > > + FF_CODEC_CAP_AUTO_THREADS | FF_CODEC_CAP_INIT_CLEANUP |
> > > > + FF_CODEC_CAP_ICC_PROFILES | FF_CODEC_CAP_EOF_FLUSH,
> > > 
> > > Why FF_CODEC_CAP_EOF_FLUSH?
> > 
> > So the encoder receives a NULL after the last frame has been submitted,
> > so JxlEncoderCloseInput can be called and the final frame properly emitted.
> 
> 
> Ah, that makes sense, thanks.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[0] https://libjxl.readthedocs.io/en/latest/api_encoder.html#_CPPv423JxlEncoderProcessOutputP10JxlEncoderPP7uint8_tP6size_t

On 12/15/23 16:31, Zsolt Vadász via ffmpeg-devel wrote:
> On Friday, December 15th, 2023 at 10:12 PM, Leo Izen <leo.izen@gmail.com> wrote:
> 
> 
>> On 12/15/23 11:40, Zsolt Vadász via ffmpeg-devel wrote:
>>
>>> On Friday, December 15th, 2023 at 12:20 AM, Leo Izen leo.izen@gmail.com wrote:
>>>
>>>>> + AVFrame *last;
>>>>
>>>> I don't see the purpose of keeping this here.
>>>
>>> The name is misleading, I should have named it `previous`, since it always contains the previous frame.
>>> I did it this way so I could call JxlEncoderCloseInput when the callback received NULL.
>>
>>
>> This isn't needed to call JxlEncoderCloseInput, as it takes one
>> argument, which is JxlEncoder *.
> 
> Indeed, but according to the docs[0]:
> "If the last frame or last box has been added, JxlEncoderCloseInput, JxlEncoderCloseFrames
> and/or JxlEncoderCloseBoxes must be called before the next JxlEncoderProcessOutput call,
> or the codestream won’t be encoded correctly."
> When the encoder eventually receives NULL, there isn't anything left to add, unless I store the previous frame,
> is there?

You can always call JxlEncoderProcessOutput at the start of the loop, 
rather than calling it after JxlEncoderAddImageFrame. (Except perhaps 
the first loop.)

If you do this, you'll process the frame that was added last loop, and 
then you can add another frame. If you receive NULL, you call 
JxlEncoderCloseInput at the start of the loop, before you call 
JxlEncoderProcessOutput. This saves you from having to store the AVFrame 
in memory. libjxl will have a copy of it in memory anyway for reference 
purposes so this allows us to reduce memory usage.


>>>>> +
>>>>> + if(!ctx->last && !avctx->internal->draining) { > + ctx->last = av_frame_clone(frame);
>>>>> + *got_packet = 0;
>>>>> + return AVERROR(EAGAIN);
>>>>
>>>> It looks like you're trying to emit one packet per image, rather than
>>>> one packet per encoded frame. This is fine, but you should not be
>>>> calling av_frame_clone, and there's no reason to use
>>>> avctx->internal->draining here. If you are doing this, you also have no
>>>> reason to cache ctx->last at all.
>>>
>>> It's the opposite, I'm trying to emit a packet for each frame of the animation.
>>
>>
>> Libjxl provides no promise of doing this meaningfully, by the way. You
>> may end up with arbitrary subdivisions, not subdivisions on frame
>> boundaries.
>>
> Well that's one thing I didn't account for, a real pity. Do you recommend emitting a
> single packet instead?

You can still accomplish what you're trying to do with libjxl but you 
need to finesse it a little bit. You need to (1) disable the container 
format, as its interaction with Frames is undefined, and you need to (2) 
loop calls to JxlEncoderProcessOutput until it returns JXL_ENC_SUCCESS.

The docs specify "This encodes the frames and/or boxes added so far" so 
it will end up flushing out the remaining frames if the container format 
is disabled.


>>>>> +const FFCodec ff_libjxl_animated_encoder = {
>>>>> + .p.name = "libjxl_animated",
>>>>> + CODEC_LONG_NAME("libjxl Animated JPEG XL"),
>>>>> + .p.type = AVMEDIA_TYPE_VIDEO,
>>>>> + .p.id = AV_CODEC_ID_JPEGXL,
>>>>> + .priv_data_size = sizeof(LibJxlEncodeContext),
>>>>> + .init = libjxl_animated_encode_init,
>>>>> + FF_CODEC_ENCODE_CB(libjxl_animated_encode_frame),
>>>>> + .close = libjxl_encode_close,
>>>>> + .p.capabilities = AV_CODEC_CAP_OTHER_THREADS |
>>>>> + AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE |
>>>>> + AV_CODEC_CAP_DELAY,
>>>>> + .caps_internal = FF_CODEC_CAP_NOT_INIT_THREADSAFE |
>>>>> + FF_CODEC_CAP_AUTO_THREADS | FF_CODEC_CAP_INIT_CLEANUP |
>>>>> + FF_CODEC_CAP_ICC_PROFILES | FF_CODEC_CAP_EOF_FLUSH,
>>>>
>>>> Why FF_CODEC_CAP_EOF_FLUSH?
>>>
>>> So the encoder receives a NULL after the last frame has been submitted,
>>> so JxlEncoderCloseInput can be called and the final frame properly emitted.
>>
>>
>> Ah, that makes sense, thanks.
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> 
> [0] https://libjxl.readthedocs.io/en/latest/api_encoder.html#_CPPv423JxlEncoderProcessOutputP10JxlEncoderPP7uint8_tP6size_t
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

On Friday, December 15th, 2023 at 11:18 PM, Leo Izen <leo.izen@gmail.com> wrote:


> On 12/15/23 16:31, Zsolt Vadász via ffmpeg-devel wrote:
> 
> > On Friday, December 15th, 2023 at 10:12 PM, Leo Izen leo.izen@gmail.com wrote:
> > 
> > > On 12/15/23 11:40, Zsolt Vadász via ffmpeg-devel wrote:
> > > 
> > > > On Friday, December 15th, 2023 at 12:20 AM, Leo Izen leo.izen@gmail.com wrote:
> > > > 
> > > > > > + AVFrame *last;
> > > > > 
> > > > > I don't see the purpose of keeping this here.
> > > > 
> > > > The name is misleading, I should have named it `previous`, since it always contains the previous frame.
> > > > I did it this way so I could call JxlEncoderCloseInput when the callback received NULL.
> > > 
> > > This isn't needed to call JxlEncoderCloseInput, as it takes one
> > > argument, which is JxlEncoder *.
> > 
> > Indeed, but according to the docs[0]:
> > "If the last frame or last box has been added, JxlEncoderCloseInput, JxlEncoderCloseFrames
> > and/or JxlEncoderCloseBoxes must be called before the next JxlEncoderProcessOutput call,
> > or the codestream won’t be encoded correctly."
> > When the encoder eventually receives NULL, there isn't anything left to add, unless I store the previous frame,
> > is there?
> 
> 
> You can always call JxlEncoderProcessOutput at the start of the loop,
> rather than calling it after JxlEncoderAddImageFrame. (Except perhaps
> the first loop.)
> 
> If you do this, you'll process the frame that was added last loop, and
> then you can add another frame. If you receive NULL, you call
> JxlEncoderCloseInput at the start of the loop, before you call
> JxlEncoderProcessOutput. This saves you from having to store the AVFrame
> in memory. libjxl will have a copy of it in memory anyway for reference
> purposes so this allows us to reduce memory usage.
> 

Thanks, this actually completely crossed my mind.
However, this complicates the PTS related parts, specifically calculating
JxlFrameHeader.duration from PTS for the first frame (since you said that
AVFrame->duration is unreliable). There'd still have to be an AVFrame
stored in the context because of it, since we don't have two PTSes yet
that we can subtract from one another. IMO it would look a tad bit cleaner
to keep the extra AVFrame in the encode context.
> > > > > > +
> > > > > > + if(!ctx->last && !avctx->internal->draining) { > + ctx->last = av_frame_clone(frame);
> > > > > > + *got_packet = 0;
> > > > > > + return AVERROR(EAGAIN);
> > > > > 
> > > > > It looks like you're trying to emit one packet per image, rather than
> > > > > one packet per encoded frame. This is fine, but you should not be
> > > > > calling av_frame_clone, and there's no reason to use
> > > > > avctx->internal->draining here. If you are doing this, you also have no
> > > > > reason to cache ctx->last at all.
> > > > 
> > > > It's the opposite, I'm trying to emit a packet for each frame of the animation.
> > > 
> > > Libjxl provides no promise of doing this meaningfully, by the way. You
> > > may end up with arbitrary subdivisions, not subdivisions on frame
> > > boundaries.
> > 
> > Well that's one thing I didn't account for, a real pity. Do you recommend emitting a
> > single packet instead?
> 
> 
> You can still accomplish what you're trying to do with libjxl but you
> need to finesse it a little bit. You need to (1) disable the container
> format, as its interaction with Frames is undefined, and you need to (2)
> loop calls to JxlEncoderProcessOutput until it returns JXL_ENC_SUCCESS.
> 
> The docs specify "This encodes the frames and/or boxes added so far" so
> it will end up flushing out the remaining frames if the container format
> is disabled.
> 
> > > > > > +const FFCodec ff_libjxl_animated_encoder = {
> > > > > > + .p.name = "libjxl_animated",
> > > > > > + CODEC_LONG_NAME("libjxl Animated JPEG XL"),
> > > > > > + .p.type = AVMEDIA_TYPE_VIDEO,
> > > > > > + .p.id = AV_CODEC_ID_JPEGXL,
> > > > > > + .priv_data_size = sizeof(LibJxlEncodeContext),
> > > > > > + .init = libjxl_animated_encode_init,
> > > > > > + FF_CODEC_ENCODE_CB(libjxl_animated_encode_frame),
> > > > > > + .close = libjxl_encode_close,
> > > > > > + .p.capabilities = AV_CODEC_CAP_OTHER_THREADS |
> > > > > > + AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE |
> > > > > > + AV_CODEC_CAP_DELAY,
> > > > > > + .caps_internal = FF_CODEC_CAP_NOT_INIT_THREADSAFE |
> > > > > > + FF_CODEC_CAP_AUTO_THREADS | FF_CODEC_CAP_INIT_CLEANUP |
> > > > > > + FF_CODEC_CAP_ICC_PROFILES | FF_CODEC_CAP_EOF_FLUSH,
> > > > > 
> > > > > Why FF_CODEC_CAP_EOF_FLUSH?
> > > > 
> > > > So the encoder receives a NULL after the last frame has been submitted,
> > > > so JxlEncoderCloseInput can be called and the final frame properly emitted.
> > > 
> > > Ah, that makes sense, thanks.
> > > _______________________________________________
> > > ffmpeg-devel mailing list
> > > ffmpeg-devel@ffmpeg.org
> > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > 
> > > To unsubscribe, visit link above, or email
> > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> > 
> > [0] https://libjxl.readthedocs.io/en/latest/api_encoder.html#_CPPv423JxlEncoderProcessOutputP10JxlEncoderPP7uint8_tP6size_t
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > 
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".