[FFmpeg-devel] [PATCH 24/46] avcodec/jpeglsenc: Avoid intermediate buffer, allow user-supplied buffers
James Almer
jamrial at gmail.com
Tue May 4 21:50:46 EEST 2021
On 4/29/2021 8:56 PM, Andreas Rheinhardt wrote:
> Up until now, the JPEG-LS encoder allocated a worst-case-sized packet
> at the beginning of each encode2 call; then it wrote the packet header
> into its destination buffer and encoded the actual packet data;
> said data is written into another worst-case-sized buffer, because it
> needs to be escaped before being written into the packet buffer.
> Finally, because the packet buffer is worst-case-sized, the generic
> code copies the actually used part into a fresh buffer.
>
> This commit changes this: Allocating the packet and writing the header
> into it is deferred until the actual data has been encoded and its size
> is known. This gives a good upper bound for the needed size of the packet
> buffer (the upper bound might be 1/15 too large) and so one can avoid the
> implicit intermediate buffer and support user-supplied buffers by using
> ff_get_encode_buffer().
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
> ---
> libavcodec/jpeglsenc.c | 102 ++++++++++++++++++++++++-----------------
> 1 file changed, 59 insertions(+), 43 deletions(-)
>
> diff --git a/libavcodec/jpeglsenc.c b/libavcodec/jpeglsenc.c
> index 17d46c0449..a7bcd78275 100644
> --- a/libavcodec/jpeglsenc.c
> +++ b/libavcodec/jpeglsenc.c
> @@ -27,6 +27,7 @@
>
> #include "avcodec.h"
> #include "bytestream.h"
> +#include "encode.h"
> #include "get_bits.h"
> #include "put_bits.h"
> #include "golomb.h"
> @@ -283,53 +284,23 @@ static int encode_picture_ls(AVCodecContext *avctx, AVPacket *pkt,
> const uint8_t *in;
> uint8_t *last = NULL;
> JLSState state = { 0 };
> - int i, size, ret;
> + size_t size;
> + int i, ret, size_in_bits;
> int comps;
>
> - if ((ret = ff_alloc_packet2(avctx, pkt, ctx->size, 0)) < 0)
> - return ret;
> -
> last = av_mallocz(FFABS(p->linesize[0]));
> if (!last)
> return AVERROR(ENOMEM);
>
> - bytestream2_init_writer(&pb, pkt->data, pkt->size);
> init_put_bits(&pb2, ctx->buf, ctx->size);
>
> - /* write our own JPEG header, can't use mjpeg_picture_header */
> comps = ctx->comps;
> - put_marker_byteu(&pb, SOI);
> - put_marker_byteu(&pb, SOF48);
> - bytestream2_put_be16u(&pb, 8 + comps * 3); // header size depends on components
> - bytestream2_put_byteu(&pb, (avctx->pix_fmt == AV_PIX_FMT_GRAY16) ? 16 : 8); // bpp
> - bytestream2_put_be16u(&pb, avctx->height);
> - bytestream2_put_be16u(&pb, avctx->width);
> - bytestream2_put_byteu(&pb, comps); // components
> - for (i = 1; i <= comps; i++) {
> - bytestream2_put_byteu(&pb, i); // component ID
> - bytestream2_put_byteu(&pb, 0x11); // subsampling: none
> - bytestream2_put_byteu(&pb, 0); // Tiq, used by JPEG-LS ext
> - }
> -
> - put_marker_byteu(&pb, SOS);
> - bytestream2_put_be16u(&pb, 6 + comps * 2);
> - bytestream2_put_byteu(&pb, comps);
> - for (i = 1; i <= comps; i++) {
> - bytestream2_put_byteu(&pb, i); // component ID
> - bytestream2_put_byteu(&pb, 0); // mapping index: none
> - }
> - bytestream2_put_byteu(&pb, ctx->pred);
> - bytestream2_put_byteu(&pb, (comps > 1) ? 1 : 0); // interleaving: 0 - plane, 1 - line
> - bytestream2_put_byteu(&pb, 0); // point transform: none
> -
> /* initialize JPEG-LS state from JPEG parameters */
> state.near = ctx->pred;
> state.bpp = (avctx->pix_fmt == AV_PIX_FMT_GRAY16) ? 16 : 8;
> ff_jpegls_reset_coding_parameters(&state, 0);
> ff_jpegls_init_state(&state);
>
> - ls_store_lse(&state, &pb);
> -
> in = p->data[0];
> if (avctx->pix_fmt == AV_PIX_FMT_GRAY8) {
> int t = 0;
> @@ -378,17 +349,63 @@ static int encode_picture_ls(AVCodecContext *avctx, AVPacket *pkt,
> in += p->linesize[0];
> }
> }
> -
> - /* the specification says that after doing 0xff escaping unused bits in
> - * the last byte must be set to 0, so just append 7 "optional" zero bits
> - * to avoid special-casing. */
> + av_free(last);
> + /* Now the actual image data has been written, which enables us to estimate
> + * the needed packet size: For every 15 input bits, an escape bit might be
> + * added below; and if put_bits_count % 15 is >= 8, then another bit might
> + * be added.
> + * Furthermore the specification says that after doing 0xff escaping unused
> + * bits in the last byte must be set to 0, so just append 7 "optional" zero
> + * bits to avoid special-casing. This also simplifies the size calculation:
> + * Properly rounding up is now automatically baked-in. */
> put_bits(&pb2, 7, 0);
> - size = put_bits_count(&pb2);
> + /* Make sure that the bit count + padding is representable in an int;
> + necessary for put_bits_count() as well as for using a GetBitContext. */
> + if (put_bytes_count(&pb2, 0) > INT_MAX / 8 - AV_INPUT_BUFFER_PADDING_SIZE)
> + return AVERROR(ERANGE);
> + size_in_bits = put_bits_count(&pb2);
> flush_put_bits(&pb2);
> + size = size_in_bits * 2U / 15;
> + size += 2 + 2 + 2 + 1 + 2 + 2 + 1 + comps * (1 + 1 + 1) + 2 + 2 + 1
> + + comps * (1 + 1) + 1 + 1 + 1; /* Header */
> + size += 2 + 2 + 1 + 2 + 2 + 2 + 2 + 2; /* LSE */
> + size += 2; /* EOI */
> + if ((ret = ff_get_encode_buffer(avctx, pkt, size, 0)) < 0)
> + return ret;
> +
> + bytestream2_init_writer(&pb, pkt->data, pkt->size);
> +
> + /* write our own JPEG header, can't use mjpeg_picture_header */
> + put_marker_byteu(&pb, SOI);
> + put_marker_byteu(&pb, SOF48);
> + bytestream2_put_be16u(&pb, 8 + comps * 3); // header size depends on components
> + bytestream2_put_byteu(&pb, (avctx->pix_fmt == AV_PIX_FMT_GRAY16) ? 16 : 8); // bpp
> + bytestream2_put_be16u(&pb, avctx->height);
> + bytestream2_put_be16u(&pb, avctx->width);
> + bytestream2_put_byteu(&pb, comps); // components
> + for (i = 1; i <= comps; i++) {
> + bytestream2_put_byteu(&pb, i); // component ID
> + bytestream2_put_byteu(&pb, 0x11); // subsampling: none
> + bytestream2_put_byteu(&pb, 0); // Tiq, used by JPEG-LS ext
> + }
> +
> + put_marker_byteu(&pb, SOS);
> + bytestream2_put_be16u(&pb, 6 + comps * 2);
> + bytestream2_put_byteu(&pb, comps);
> + for (i = 1; i <= comps; i++) {
> + bytestream2_put_byteu(&pb, i); // component ID
> + bytestream2_put_byteu(&pb, 0); // mapping index: none
> + }
> + bytestream2_put_byteu(&pb, ctx->pred);
> + bytestream2_put_byteu(&pb, (comps > 1) ? 1 : 0); // interleaving: 0 - plane, 1 - line
> + bytestream2_put_byteu(&pb, 0); // point transform: none
> +
> + ls_store_lse(&state, &pb);
> +
> /* do escape coding */
> - init_get_bits(&gb, pb2.buf, size);
> - size -= 7;
> - while (get_bits_count(&gb) < size) {
> + init_get_bits(&gb, pb2.buf, size_in_bits);
> + size_in_bits -= 7;
> + while (get_bits_count(&gb) < size_in_bits) {
> int v;
> v = get_bits(&gb, 8);
> bytestream2_put_byte(&pb, v);
> @@ -397,15 +414,14 @@ static int encode_picture_ls(AVCodecContext *avctx, AVPacket *pkt,
> bytestream2_put_byte(&pb, v);
> }
> }
> - av_freep(&last);
>
> /* End of image */
> put_marker_byte(&pb, EOI);
>
> emms_c();
>
> - pkt->size = bytestream2_tell_p(&pb);
> pkt->flags |= AV_PKT_FLAG_KEY;
> + av_shrink_packet(pkt, bytestream2_tell_p(&pb));
You're shrinking the packet allocated by ff_get_encode_buffer(). Do we
really want that? The doxy for AVCodec.get_encode_buffer() does not say
that the requested size is not final and may change. It is obviously
implicit that it will not grow, but the user may not expect it to shrink
either.
Think an scenario like having a single buffer meant to be propagated
through the network, where the user writes a header at a given offset,
sets pkt->data in their custom get_encode_buffer() implementation to an
offset immediately after said header, and then keeps the offset for the
next packet around (data + size) to be used in the next
get_encode_buffer() call. If pkt->size changes when the packet is
returned by avcodec_receive_frame(), this would break.
Do you think it's worth amending the doxy with a comment that pkt->size
may change (shrink) during the process after get_encode_buffer()? Or
internally ensure no encoder changes it instead? Or is it too much of an
obscure scenario and the user should only consider the returned packet
size as the actual final one?
> *got_packet = 1;
> return 0;
> }
> @@ -468,9 +484,9 @@ const AVCodec ff_jpegls_encoder = {
> .long_name = NULL_IF_CONFIG_SMALL("JPEG-LS"),
> .type = AVMEDIA_TYPE_VIDEO,
> .id = AV_CODEC_ID_JPEGLS,
> + .capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_FRAME_THREADS,
> .priv_data_size = sizeof(JPEGLSContext),
> .priv_class = &jpegls_class,
> - .capabilities = AV_CODEC_CAP_FRAME_THREADS,
> .init = encode_jpegls_init,
> .encode2 = encode_picture_ls,
> .close = encode_jpegls_close,
>
More information about the ffmpeg-devel
mailing list