* [FFmpeg-devel] [PATCH] avcodec/libaomenc: remove one memcpy when queueing packets
@ 2022-08-24 22:52 James Almer
2022-08-25 16:29 ` Andreas Rheinhardt
0 siblings, 1 reply; 4+ messages in thread
From: James Almer @ 2022-08-24 22:52 UTC (permalink / raw)
To: ffmpeg-devel
Don't use an intermediary buffer. Achieve this by replacing FrameListData with
a PacketList, and by allocating and populating every packet's payload before
inserting them into the list.
Signed-off-by: James Almer <jamrial@gmail.com>
---
libavcodec/libaomenc.c | 195 +++++++++++++++--------------------------
1 file changed, 70 insertions(+), 125 deletions(-)
diff --git a/libavcodec/libaomenc.c b/libavcodec/libaomenc.c
index 485f554165..f9476b3ddf 100644
--- a/libavcodec/libaomenc.c
+++ b/libavcodec/libaomenc.c
@@ -38,6 +38,7 @@
#include "av1.h"
#include "avcodec.h"
+#include "bytestream.h"
#include "bsf.h"
#include "codec_internal.h"
#include "encode.h"
@@ -46,24 +47,6 @@
#include "packet_internal.h"
#include "profiles.h"
-/*
- * Portion of struct aom_codec_cx_pkt from aom_encoder.h.
- * One encoded frame returned from the library.
- */
-struct FrameListData {
- void *buf; /**< compressed data buffer */
- size_t sz; /**< length of compressed data */
- int64_t pts; /**< time stamp to show frame
- (in timebase units) */
- unsigned long duration; /**< duration to show frame
- (in timebase units) */
- uint32_t flags; /**< flags for this frame */
- uint64_t sse[4];
- int have_sse; /**< true if we have pending sse[] */
- uint64_t frame_number;
- struct FrameListData *next;
-};
-
typedef struct AOMEncoderContext {
AVClass *class;
AVBSFContext *bsf;
@@ -71,7 +54,8 @@ typedef struct AOMEncoderContext {
struct aom_image rawimg;
struct aom_fixed_buf twopass_stats;
unsigned twopass_stats_size;
- struct FrameListData *coded_frame_list;
+ PacketList coded_frame_list;
+ AVPacket *pkt;
int cpu_used;
int auto_alt_ref;
int arnr_max_frames;
@@ -283,33 +267,6 @@ static av_cold void dump_enc_cfg(AVCodecContext *avctx,
av_log(avctx, level, "\n");
}
-static void coded_frame_add(void *list, struct FrameListData *cx_frame)
-{
- struct FrameListData **p = list;
-
- while (*p)
- p = &(*p)->next;
- *p = cx_frame;
- cx_frame->next = NULL;
-}
-
-static av_cold void free_coded_frame(struct FrameListData *cx_frame)
-{
- av_freep(&cx_frame->buf);
- av_freep(&cx_frame);
-}
-
-static av_cold void free_frame_list(struct FrameListData *list)
-{
- struct FrameListData *p = list;
-
- while (p) {
- list = list->next;
- free_coded_frame(p);
- p = list;
- }
-}
-
static av_cold int codecctl_int(AVCodecContext *avctx,
#ifdef UENUM1BYTE
aome_enc_control_id id,
@@ -432,7 +389,8 @@ static av_cold int aom_free(AVCodecContext *avctx)
aom_codec_destroy(&ctx->encoder);
av_freep(&ctx->twopass_stats.buf);
av_freep(&avctx->stats_out);
- free_frame_list(ctx->coded_frame_list);
+ avpriv_packet_list_free(&ctx->coded_frame_list);
+ av_packet_free(&ctx->pkt);
av_bsf_free(&ctx->bsf);
return 0;
}
@@ -1042,6 +1000,10 @@ static av_cold int aom_init(AVCodecContext *avctx,
return ret;
}
+ ctx->pkt = av_packet_alloc();
+ if (!ctx->pkt)
+ return AVERROR(ENOMEM);
+
if (enccfg.rc_end_usage == AOM_CBR ||
enccfg.g_pass != AOM_RC_ONE_PASS) {
cpb_props->max_bitrate = avctx->rc_max_rate;
@@ -1053,25 +1015,40 @@ static av_cold int aom_init(AVCodecContext *avctx,
return 0;
}
-static inline void cx_pktcpy(AOMContext *ctx,
- struct FrameListData *dst,
+static inline int cx_pktcpy(AVCodecContext *avctx,
+ AVPacket *dst,
const struct aom_codec_cx_pkt *src)
{
- dst->pts = src->data.frame.pts;
- dst->duration = src->data.frame.duration;
- dst->flags = src->data.frame.flags;
- dst->sz = src->data.frame.sz;
- dst->buf = src->data.frame.buf;
+ AOMContext *ctx = avctx->priv_data;
+ int av_unused pict_type;
+ int ret;
+
+ av_packet_unref(dst);
+ ret = ff_get_encode_buffer(avctx, dst, src->data.frame.sz, 0);
+ if (ret < 0) {
+ av_log(avctx, AV_LOG_ERROR,
+ "Error getting output packet of size %"SIZE_SPECIFIER".\n", src->data.frame.sz);
+ return ret;
+ }
+ memcpy(dst->data, src->data.frame.buf, src->data.frame.sz);
+ dst->pts = dst->dts = src->data.frame.pts;
+
+ if (src->data.frame.flags & AOM_FRAME_IS_KEY) {
+ dst->flags |= AV_PKT_FLAG_KEY;
#ifdef AOM_FRAME_IS_INTRAONLY
- dst->frame_number = ++ctx->frame_number;
- dst->have_sse = ctx->have_sse;
+ pict_type = AV_PICTURE_TYPE_I;
+ } else if (src->data.frame.flags & AOM_FRAME_IS_INTRAONLY) {
+ pict_type = AV_PICTURE_TYPE_I;
+ } else {
+ pict_type = AV_PICTURE_TYPE_P;
+ }
+
if (ctx->have_sse) {
- /* associate last-seen SSE to the frame. */
- /* Transfers ownership from ctx to dst. */
- memcpy(dst->sse, ctx->sse, sizeof(dst->sse));
+ ff_side_data_set_encoder_stats(dst, 0, ctx->sse + 1, 3, pict_type);
ctx->have_sse = 0;
- }
#endif
+ }
+ return 0;
}
/**
@@ -1081,50 +1058,32 @@ static inline void cx_pktcpy(AOMContext *ctx,
* @return packet data size on success
* @return a negative AVERROR on error
*/
-static int storeframe(AVCodecContext *avctx, struct FrameListData *cx_frame,
- AVPacket *pkt)
+static int storeframe(AVCodecContext *avctx, AVPacket *dst, AVPacket *src)
{
AOMContext *ctx = avctx->priv_data;
- int av_unused pict_type;
- int ret = ff_get_encode_buffer(avctx, pkt, cx_frame->sz, 0);
- if (ret < 0) {
- av_log(avctx, AV_LOG_ERROR,
- "Error getting output packet of size %"SIZE_SPECIFIER".\n", cx_frame->sz);
- return ret;
- }
- memcpy(pkt->data, cx_frame->buf, pkt->size);
- pkt->pts = pkt->dts = cx_frame->pts;
+ const uint8_t *sd;
+ size_t size;
+ int ret;
- if (!!(cx_frame->flags & AOM_FRAME_IS_KEY)) {
- pkt->flags |= AV_PKT_FLAG_KEY;
-#ifdef AOM_FRAME_IS_INTRAONLY
- pict_type = AV_PICTURE_TYPE_I;
- } else if (cx_frame->flags & AOM_FRAME_IS_INTRAONLY) {
- pict_type = AV_PICTURE_TYPE_I;
- } else {
- pict_type = AV_PICTURE_TYPE_P;
- }
-
- ff_side_data_set_encoder_stats(pkt, 0, cx_frame->sse + 1,
- cx_frame->have_sse ? 3 : 0, pict_type);
+ av_packet_move_ref(dst, src);
- if (cx_frame->have_sse) {
+ sd = av_packet_get_side_data(dst, AV_PKT_DATA_QUALITY_STATS, &size);
+ if (sd && size >= 4 + 4 + 8 * 3) {
int i;
+ sd += 4 + 4;
for (i = 0; i < 3; ++i) {
- avctx->error[i] += cx_frame->sse[i + 1];
+ avctx->error[i] += bytestream_get_le64(&sd);
}
- cx_frame->have_sse = 0;
-#endif
}
if (avctx->flags & AV_CODEC_FLAG_GLOBAL_HEADER) {
- ret = av_bsf_send_packet(ctx->bsf, pkt);
+ ret = av_bsf_send_packet(ctx->bsf, dst);
if (ret < 0) {
av_log(avctx, AV_LOG_ERROR, "extract_extradata filter "
"failed to send input packet\n");
return ret;
}
- ret = av_bsf_receive_packet(ctx->bsf, pkt);
+ ret = av_bsf_receive_packet(ctx->bsf, dst);
if (ret < 0) {
av_log(avctx, AV_LOG_ERROR, "extract_extradata filter "
@@ -1132,7 +1091,7 @@ static int storeframe(AVCodecContext *avctx, struct FrameListData *cx_frame,
return ret;
}
}
- return pkt->size;
+ return dst->size;
}
/**
@@ -1148,16 +1107,14 @@ static int queue_frames(AVCodecContext *avctx, AVPacket *pkt_out)
AOMContext *ctx = avctx->priv_data;
const struct aom_codec_cx_pkt *pkt;
const void *iter = NULL;
- int size = 0;
+ int ret, size = 0;
- if (ctx->coded_frame_list) {
- struct FrameListData *cx_frame = ctx->coded_frame_list;
+ if (!avpriv_packet_list_get(&ctx->coded_frame_list, ctx->pkt)) {
/* return the leading frame if we've already begun queueing */
- size = storeframe(avctx, cx_frame, pkt_out);
- if (size < 0)
- return size;
- ctx->coded_frame_list = cx_frame->next;
- free_coded_frame(cx_frame);
+ ret = storeframe(avctx, pkt_out, ctx->pkt);
+ if (ret < 0)
+ goto fail;
+ size = ret;
}
/* consume all available output from the encoder before returning. buffers
@@ -1165,37 +1122,21 @@ static int queue_frames(AVCodecContext *avctx, AVPacket *pkt_out)
while ((pkt = aom_codec_get_cx_data(&ctx->encoder, &iter))) {
switch (pkt->kind) {
case AOM_CODEC_CX_FRAME_PKT:
+ ret = cx_pktcpy(avctx, ctx->pkt, pkt);
+ if (ret < 0)
+ goto fail;
if (!size) {
- struct FrameListData cx_frame;
-
/* avoid storing the frame when the list is empty and we haven't yet
* provided a frame for output */
- av_assert0(!ctx->coded_frame_list);
- cx_pktcpy(ctx, &cx_frame, pkt);
- size = storeframe(avctx, &cx_frame, pkt_out);
- if (size < 0)
- return size;
+ av_assert0(!ctx->coded_frame_list.head);
+ ret = storeframe(avctx, pkt_out, ctx->pkt);
+ if (ret < 0)
+ goto fail;
+ size = ret;
} else {
- struct FrameListData *cx_frame =
- av_malloc(sizeof(struct FrameListData));
-
- if (!cx_frame) {
- av_log(avctx, AV_LOG_ERROR,
- "Frame queue element alloc failed\n");
- return AVERROR(ENOMEM);
- }
- cx_pktcpy(ctx, cx_frame, pkt);
- cx_frame->buf = av_malloc(cx_frame->sz);
-
- if (!cx_frame->buf) {
- av_log(avctx, AV_LOG_ERROR,
- "Data buffer alloc (%"SIZE_SPECIFIER" bytes) failed\n",
- cx_frame->sz);
- av_freep(&cx_frame);
- return AVERROR(ENOMEM);
- }
- memcpy(cx_frame->buf, pkt->data.frame.buf, pkt->data.frame.sz);
- coded_frame_add(&ctx->coded_frame_list, cx_frame);
+ ret = avpriv_packet_list_put(&ctx->coded_frame_list, ctx->pkt, NULL, 0);
+ if (ret < 0)
+ goto fail;
}
break;
case AOM_CODEC_STATS_PKT:
@@ -1236,6 +1177,10 @@ static int queue_frames(AVCodecContext *avctx, AVPacket *pkt_out)
}
return size;
+fail:
+ av_packet_unref(ctx->pkt);
+ av_packet_unref(pkt_out);
+ return ret;
}
static enum AVPixelFormat aomfmt_to_pixfmt(struct aom_image *img)
--
2.37.2
_______________________________________________
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".
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avcodec/libaomenc: remove one memcpy when queueing packets
2022-08-24 22:52 [FFmpeg-devel] [PATCH] avcodec/libaomenc: remove one memcpy when queueing packets James Almer
@ 2022-08-25 16:29 ` Andreas Rheinhardt
2022-08-25 16:56 ` James Almer
2022-08-25 17:46 ` [FFmpeg-devel] [PATCH v2] " James Almer
0 siblings, 2 replies; 4+ messages in thread
From: Andreas Rheinhardt @ 2022-08-25 16:29 UTC (permalink / raw)
To: ffmpeg-devel
James Almer:
> Don't use an intermediary buffer. Achieve this by replacing FrameListData with
> a PacketList, and by allocating and populating every packet's payload before
> inserting them into the list.
>
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> libavcodec/libaomenc.c | 195 +++++++++++++++--------------------------
> 1 file changed, 70 insertions(+), 125 deletions(-)
>
> diff --git a/libavcodec/libaomenc.c b/libavcodec/libaomenc.c
> index 485f554165..f9476b3ddf 100644
> --- a/libavcodec/libaomenc.c
> +++ b/libavcodec/libaomenc.c
> @@ -38,6 +38,7 @@
>
> #include "av1.h"
> #include "avcodec.h"
> +#include "bytestream.h"
> #include "bsf.h"
> #include "codec_internal.h"
> #include "encode.h"
> @@ -46,24 +47,6 @@
> #include "packet_internal.h"
> #include "profiles.h"
>
> -/*
> - * Portion of struct aom_codec_cx_pkt from aom_encoder.h.
> - * One encoded frame returned from the library.
> - */
> -struct FrameListData {
> - void *buf; /**< compressed data buffer */
> - size_t sz; /**< length of compressed data */
> - int64_t pts; /**< time stamp to show frame
> - (in timebase units) */
> - unsigned long duration; /**< duration to show frame
> - (in timebase units) */
> - uint32_t flags; /**< flags for this frame */
> - uint64_t sse[4];
> - int have_sse; /**< true if we have pending sse[] */
> - uint64_t frame_number;
> - struct FrameListData *next;
> -};
> -
> typedef struct AOMEncoderContext {
> AVClass *class;
> AVBSFContext *bsf;
> @@ -71,7 +54,8 @@ typedef struct AOMEncoderContext {
> struct aom_image rawimg;
> struct aom_fixed_buf twopass_stats;
> unsigned twopass_stats_size;
> - struct FrameListData *coded_frame_list;
> + PacketList coded_frame_list;
> + AVPacket *pkt;
Renaming this variable to avpkt would improve clarity by simplifying
distinguishing it from the aom_codec_cx_pkt packets.
> int cpu_used;
> int auto_alt_ref;
> int arnr_max_frames;
> @@ -283,33 +267,6 @@ static av_cold void dump_enc_cfg(AVCodecContext *avctx,
> av_log(avctx, level, "\n");
> }
>
> -static void coded_frame_add(void *list, struct FrameListData *cx_frame)
> -{
> - struct FrameListData **p = list;
> -
> - while (*p)
> - p = &(*p)->next;
> - *p = cx_frame;
> - cx_frame->next = NULL;
> -}
> -
> -static av_cold void free_coded_frame(struct FrameListData *cx_frame)
> -{
> - av_freep(&cx_frame->buf);
> - av_freep(&cx_frame);
> -}
> -
> -static av_cold void free_frame_list(struct FrameListData *list)
> -{
> - struct FrameListData *p = list;
> -
> - while (p) {
> - list = list->next;
> - free_coded_frame(p);
> - p = list;
> - }
> -}
> -
> static av_cold int codecctl_int(AVCodecContext *avctx,
> #ifdef UENUM1BYTE
> aome_enc_control_id id,
> @@ -432,7 +389,8 @@ static av_cold int aom_free(AVCodecContext *avctx)
> aom_codec_destroy(&ctx->encoder);
> av_freep(&ctx->twopass_stats.buf);
> av_freep(&avctx->stats_out);
> - free_frame_list(ctx->coded_frame_list);
> + avpriv_packet_list_free(&ctx->coded_frame_list);
> + av_packet_free(&ctx->pkt);
> av_bsf_free(&ctx->bsf);
> return 0;
> }
> @@ -1042,6 +1000,10 @@ static av_cold int aom_init(AVCodecContext *avctx,
> return ret;
> }
>
> + ctx->pkt = av_packet_alloc();
> + if (!ctx->pkt)
> + return AVERROR(ENOMEM);
> +
This encoder does not have the INIT_CLEANUP flag set, so everything
leaks in case the above allocation fails. In fact, it seems like there
are already leaks in several errors paths in this function.
> if (enccfg.rc_end_usage == AOM_CBR ||
> enccfg.g_pass != AOM_RC_ONE_PASS) {
> cpb_props->max_bitrate = avctx->rc_max_rate;
> @@ -1053,25 +1015,40 @@ static av_cold int aom_init(AVCodecContext *avctx,
> return 0;
> }
>
> -static inline void cx_pktcpy(AOMContext *ctx,
> - struct FrameListData *dst,
> +static inline int cx_pktcpy(AVCodecContext *avctx,
We should not override the compiler's inlining behaviour unless we have
a good reason to do so, so you could remove it while at it.
> + AVPacket *dst,
Wrong indentation.
> const struct aom_codec_cx_pkt *src)
> {
> - dst->pts = src->data.frame.pts;
> - dst->duration = src->data.frame.duration;
> - dst->flags = src->data.frame.flags;
> - dst->sz = src->data.frame.sz;
> - dst->buf = src->data.frame.buf;
> + AOMContext *ctx = avctx->priv_data;
> + int av_unused pict_type;
> + int ret;
> +
> + av_packet_unref(dst);
Can dst ever be non-blank here (i.e. before the unref)?
> + ret = ff_get_encode_buffer(avctx, dst, src->data.frame.sz, 0);
> + if (ret < 0) {
> + av_log(avctx, AV_LOG_ERROR,
> + "Error getting output packet of size %"SIZE_SPECIFIER".\n", src->data.frame.sz);
> + return ret;
> + }
> + memcpy(dst->data, src->data.frame.buf, src->data.frame.sz);
> + dst->pts = dst->dts = src->data.frame.pts;
> +
> + if (src->data.frame.flags & AOM_FRAME_IS_KEY) {
> + dst->flags |= AV_PKT_FLAG_KEY;
> #ifdef AOM_FRAME_IS_INTRAONLY
> - dst->frame_number = ++ctx->frame_number;
> - dst->have_sse = ctx->have_sse;
> + pict_type = AV_PICTURE_TYPE_I;
> + } else if (src->data.frame.flags & AOM_FRAME_IS_INTRAONLY) {
> + pict_type = AV_PICTURE_TYPE_I;
> + } else {
> + pict_type = AV_PICTURE_TYPE_P;
> + }
> +
> if (ctx->have_sse) {
> - /* associate last-seen SSE to the frame. */
> - /* Transfers ownership from ctx to dst. */
> - memcpy(dst->sse, ctx->sse, sizeof(dst->sse));
> + ff_side_data_set_encoder_stats(dst, 0, ctx->sse + 1, 3, pict_type);
This function can fail.
> ctx->have_sse = 0;
> - }
> #endif
> + }
> + return 0;
> }
>
> /**
> @@ -1081,50 +1058,32 @@ static inline void cx_pktcpy(AOMContext *ctx,
> * @return packet data size on success
> * @return a negative AVERROR on error
> */
> -static int storeframe(AVCodecContext *avctx, struct FrameListData *cx_frame,
> - AVPacket *pkt)
> +static int storeframe(AVCodecContext *avctx, AVPacket *dst, AVPacket *src)
> {
> AOMContext *ctx = avctx->priv_data;
> - int av_unused pict_type;
> - int ret = ff_get_encode_buffer(avctx, pkt, cx_frame->sz, 0);
> - if (ret < 0) {
> - av_log(avctx, AV_LOG_ERROR,
> - "Error getting output packet of size %"SIZE_SPECIFIER".\n", cx_frame->sz);
> - return ret;
> - }
> - memcpy(pkt->data, cx_frame->buf, pkt->size);
> - pkt->pts = pkt->dts = cx_frame->pts;
> + const uint8_t *sd;
> + size_t size;
> + int ret;
>
> - if (!!(cx_frame->flags & AOM_FRAME_IS_KEY)) {
> - pkt->flags |= AV_PKT_FLAG_KEY;
> -#ifdef AOM_FRAME_IS_INTRAONLY
> - pict_type = AV_PICTURE_TYPE_I;
> - } else if (cx_frame->flags & AOM_FRAME_IS_INTRAONLY) {
> - pict_type = AV_PICTURE_TYPE_I;
> - } else {
> - pict_type = AV_PICTURE_TYPE_P;
> - }
> -
> - ff_side_data_set_encoder_stats(pkt, 0, cx_frame->sse + 1,
> - cx_frame->have_sse ? 3 : 0, pict_type);
> + av_packet_move_ref(dst, src);
>
> - if (cx_frame->have_sse) {
> + sd = av_packet_get_side_data(dst, AV_PKT_DATA_QUALITY_STATS, &size);
> + if (sd && size >= 4 + 4 + 8 * 3) {
> int i;
> + sd += 4 + 4;
> for (i = 0; i < 3; ++i) {
> - avctx->error[i] += cx_frame->sse[i + 1];
> + avctx->error[i] += bytestream_get_le64(&sd);
> }
> - cx_frame->have_sse = 0;
> -#endif
> }
>
> if (avctx->flags & AV_CODEC_FLAG_GLOBAL_HEADER) {
> - ret = av_bsf_send_packet(ctx->bsf, pkt);
> + ret = av_bsf_send_packet(ctx->bsf, dst);
> if (ret < 0) {
> av_log(avctx, AV_LOG_ERROR, "extract_extradata filter "
> "failed to send input packet\n");
> return ret;
> }
> - ret = av_bsf_receive_packet(ctx->bsf, pkt);
> + ret = av_bsf_receive_packet(ctx->bsf, dst);
>
> if (ret < 0) {
> av_log(avctx, AV_LOG_ERROR, "extract_extradata filter "
> @@ -1132,7 +1091,7 @@ static int storeframe(AVCodecContext *avctx, struct FrameListData *cx_frame,
> return ret;
> }
> }
> - return pkt->size;
> + return dst->size;
> }
>
> /**
> @@ -1148,16 +1107,14 @@ static int queue_frames(AVCodecContext *avctx, AVPacket *pkt_out)
> AOMContext *ctx = avctx->priv_data;
> const struct aom_codec_cx_pkt *pkt;
> const void *iter = NULL;
> - int size = 0;
> + int ret, size = 0;
>
> - if (ctx->coded_frame_list) {
> - struct FrameListData *cx_frame = ctx->coded_frame_list;
> + if (!avpriv_packet_list_get(&ctx->coded_frame_list, ctx->pkt)) {
> /* return the leading frame if we've already begun queueing */
> - size = storeframe(avctx, cx_frame, pkt_out);
> - if (size < 0)
> - return size;
> - ctx->coded_frame_list = cx_frame->next;
> - free_coded_frame(cx_frame);
> + ret = storeframe(avctx, pkt_out, ctx->pkt);
> + if (ret < 0)
> + goto fail;
> + size = ret;
> }
>
> /* consume all available output from the encoder before returning. buffers
> @@ -1165,37 +1122,21 @@ static int queue_frames(AVCodecContext *avctx, AVPacket *pkt_out)
> while ((pkt = aom_codec_get_cx_data(&ctx->encoder, &iter))) {
> switch (pkt->kind) {
> case AOM_CODEC_CX_FRAME_PKT:
> + ret = cx_pktcpy(avctx, ctx->pkt, pkt);
> + if (ret < 0)
> + goto fail;
> if (!size) {
> - struct FrameListData cx_frame;
> -
> /* avoid storing the frame when the list is empty and we haven't yet
> * provided a frame for output */
> - av_assert0(!ctx->coded_frame_list);
> - cx_pktcpy(ctx, &cx_frame, pkt);
> - size = storeframe(avctx, &cx_frame, pkt_out);
> - if (size < 0)
> - return size;
> + av_assert0(!ctx->coded_frame_list.head);
> + ret = storeframe(avctx, pkt_out, ctx->pkt);
> + if (ret < 0)
> + goto fail;
> + size = ret;
> } else {
> - struct FrameListData *cx_frame =
> - av_malloc(sizeof(struct FrameListData));
> -
> - if (!cx_frame) {
> - av_log(avctx, AV_LOG_ERROR,
> - "Frame queue element alloc failed\n");
> - return AVERROR(ENOMEM);
> - }
> - cx_pktcpy(ctx, cx_frame, pkt);
> - cx_frame->buf = av_malloc(cx_frame->sz);
> -
> - if (!cx_frame->buf) {
> - av_log(avctx, AV_LOG_ERROR,
> - "Data buffer alloc (%"SIZE_SPECIFIER" bytes) failed\n",
> - cx_frame->sz);
> - av_freep(&cx_frame);
> - return AVERROR(ENOMEM);
> - }
> - memcpy(cx_frame->buf, pkt->data.frame.buf, pkt->data.frame.sz);
I am shocked to see that there were two memcpies.
> - coded_frame_add(&ctx->coded_frame_list, cx_frame);
> + ret = avpriv_packet_list_put(&ctx->coded_frame_list, ctx->pkt, NULL, 0);
> + if (ret < 0)
> + goto fail;
wtf: Any error that queue_frames() returns will be translated to
"got_packet = 1" by the caller (with return code 0). Error handling in
this encoder seems to be a joke.
> }
> break;
> case AOM_CODEC_STATS_PKT:
> @@ -1236,6 +1177,10 @@ static int queue_frames(AVCodecContext *avctx, AVPacket *pkt_out)
> }
>
> return size;
> +fail:
> + av_packet_unref(ctx->pkt);
> + av_packet_unref(pkt_out);
> + return ret;
> }
>
> static enum AVPixelFormat aomfmt_to_pixfmt(struct aom_image *img)
_______________________________________________
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".
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avcodec/libaomenc: remove one memcpy when queueing packets
2022-08-25 16:29 ` Andreas Rheinhardt
@ 2022-08-25 16:56 ` James Almer
2022-08-25 17:46 ` [FFmpeg-devel] [PATCH v2] " James Almer
1 sibling, 0 replies; 4+ messages in thread
From: James Almer @ 2022-08-25 16:56 UTC (permalink / raw)
To: ffmpeg-devel
On 8/25/2022 1:29 PM, Andreas Rheinhardt wrote:
> James Almer:
>> Don't use an intermediary buffer. Achieve this by replacing FrameListData with
>> a PacketList, and by allocating and populating every packet's payload before
>> inserting them into the list.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> libavcodec/libaomenc.c | 195 +++++++++++++++--------------------------
>> 1 file changed, 70 insertions(+), 125 deletions(-)
>>
>> diff --git a/libavcodec/libaomenc.c b/libavcodec/libaomenc.c
>> index 485f554165..f9476b3ddf 100644
>> --- a/libavcodec/libaomenc.c
>> +++ b/libavcodec/libaomenc.c
>> @@ -38,6 +38,7 @@
>>
>> #include "av1.h"
>> #include "avcodec.h"
>> +#include "bytestream.h"
>> #include "bsf.h"
>> #include "codec_internal.h"
>> #include "encode.h"
>> @@ -46,24 +47,6 @@
>> #include "packet_internal.h"
>> #include "profiles.h"
>>
>> -/*
>> - * Portion of struct aom_codec_cx_pkt from aom_encoder.h.
>> - * One encoded frame returned from the library.
>> - */
>> -struct FrameListData {
>> - void *buf; /**< compressed data buffer */
>> - size_t sz; /**< length of compressed data */
>> - int64_t pts; /**< time stamp to show frame
>> - (in timebase units) */
>> - unsigned long duration; /**< duration to show frame
>> - (in timebase units) */
>> - uint32_t flags; /**< flags for this frame */
>> - uint64_t sse[4];
>> - int have_sse; /**< true if we have pending sse[] */
>> - uint64_t frame_number;
>> - struct FrameListData *next;
>> -};
>> -
>> typedef struct AOMEncoderContext {
>> AVClass *class;
>> AVBSFContext *bsf;
>> @@ -71,7 +54,8 @@ typedef struct AOMEncoderContext {
>> struct aom_image rawimg;
>> struct aom_fixed_buf twopass_stats;
>> unsigned twopass_stats_size;
>> - struct FrameListData *coded_frame_list;
>> + PacketList coded_frame_list;
>> + AVPacket *pkt;
>
> Renaming this variable to avpkt would improve clarity by simplifying
> distinguishing it from the aom_codec_cx_pkt packets.
Ok.
>
>> int cpu_used;
>> int auto_alt_ref;
>> int arnr_max_frames;
>> @@ -283,33 +267,6 @@ static av_cold void dump_enc_cfg(AVCodecContext *avctx,
>> av_log(avctx, level, "\n");
>> }
>>
>> -static void coded_frame_add(void *list, struct FrameListData *cx_frame)
>> -{
>> - struct FrameListData **p = list;
>> -
>> - while (*p)
>> - p = &(*p)->next;
>> - *p = cx_frame;
>> - cx_frame->next = NULL;
>> -}
>> -
>> -static av_cold void free_coded_frame(struct FrameListData *cx_frame)
>> -{
>> - av_freep(&cx_frame->buf);
>> - av_freep(&cx_frame);
>> -}
>> -
>> -static av_cold void free_frame_list(struct FrameListData *list)
>> -{
>> - struct FrameListData *p = list;
>> -
>> - while (p) {
>> - list = list->next;
>> - free_coded_frame(p);
>> - p = list;
>> - }
>> -}
>> -
>> static av_cold int codecctl_int(AVCodecContext *avctx,
>> #ifdef UENUM1BYTE
>> aome_enc_control_id id,
>> @@ -432,7 +389,8 @@ static av_cold int aom_free(AVCodecContext *avctx)
>> aom_codec_destroy(&ctx->encoder);
>> av_freep(&ctx->twopass_stats.buf);
>> av_freep(&avctx->stats_out);
>> - free_frame_list(ctx->coded_frame_list);
>> + avpriv_packet_list_free(&ctx->coded_frame_list);
>> + av_packet_free(&ctx->pkt);
>> av_bsf_free(&ctx->bsf);
>> return 0;
>> }
>> @@ -1042,6 +1000,10 @@ static av_cold int aom_init(AVCodecContext *avctx,
>> return ret;
>> }
>>
>> + ctx->pkt = av_packet_alloc();
>> + if (!ctx->pkt)
>> + return AVERROR(ENOMEM);
>> +
>
> This encoder does not have the INIT_CLEANUP flag set, so everything
> leaks in case the above allocation fails. In fact, it seems like there
> are already leaks in several errors paths in this function.
Will add that flag in a separate patch.
>
>> if (enccfg.rc_end_usage == AOM_CBR ||
>> enccfg.g_pass != AOM_RC_ONE_PASS) {
>> cpb_props->max_bitrate = avctx->rc_max_rate;
>> @@ -1053,25 +1015,40 @@ static av_cold int aom_init(AVCodecContext *avctx,
>> return 0;
>> }
>>
>> -static inline void cx_pktcpy(AOMContext *ctx,
>> - struct FrameListData *dst,
>> +static inline int cx_pktcpy(AVCodecContext *avctx,
>
> We should not override the compiler's inlining behaviour unless we have
> a good reason to do so, so you could remove it while at it.
Ok.
>
>> + AVPacket *dst,
>
> Wrong indentation.
Will fix. And while at it align the line below too.
>
>> const struct aom_codec_cx_pkt *src)
>> {
>> - dst->pts = src->data.frame.pts;
>> - dst->duration = src->data.frame.duration;
>> - dst->flags = src->data.frame.flags;
>> - dst->sz = src->data.frame.sz;
>> - dst->buf = src->data.frame.buf;
>> + AOMContext *ctx = avctx->priv_data;
>> + int av_unused pict_type;
>> + int ret;
>> +
>> + av_packet_unref(dst);
>
> Can dst ever be non-blank here (i.e. before the unref)?
Don't think so. It's probably a remnant from before i added the unref at
the end of queue_frames().
Will remove it.
>
>> + ret = ff_get_encode_buffer(avctx, dst, src->data.frame.sz, 0);
>> + if (ret < 0) {
>> + av_log(avctx, AV_LOG_ERROR,
>> + "Error getting output packet of size %"SIZE_SPECIFIER".\n", src->data.frame.sz);
>> + return ret;
>> + }
>> + memcpy(dst->data, src->data.frame.buf, src->data.frame.sz);
>> + dst->pts = dst->dts = src->data.frame.pts;
>> +
>> + if (src->data.frame.flags & AOM_FRAME_IS_KEY) {
>> + dst->flags |= AV_PKT_FLAG_KEY;
>> #ifdef AOM_FRAME_IS_INTRAONLY
>> - dst->frame_number = ++ctx->frame_number;
>> - dst->have_sse = ctx->have_sse;
>> + pict_type = AV_PICTURE_TYPE_I;
>> + } else if (src->data.frame.flags & AOM_FRAME_IS_INTRAONLY) {
>> + pict_type = AV_PICTURE_TYPE_I;
>> + } else {
>> + pict_type = AV_PICTURE_TYPE_P;
>> + }
>> +
>> if (ctx->have_sse) {
>> - /* associate last-seen SSE to the frame. */
>> - /* Transfers ownership from ctx to dst. */
>> - memcpy(dst->sse, ctx->sse, sizeof(dst->sse));
>> + ff_side_data_set_encoder_stats(dst, 0, ctx->sse + 1, 3, pict_type);
>
> This function can fail.
Will add a check.
>
>> ctx->have_sse = 0;
>> - }
>> #endif
>> + }
>> + return 0;
>> }
>>
>> /**
>> @@ -1081,50 +1058,32 @@ static inline void cx_pktcpy(AOMContext *ctx,
>> * @return packet data size on success
>> * @return a negative AVERROR on error
>> */
>> -static int storeframe(AVCodecContext *avctx, struct FrameListData *cx_frame,
>> - AVPacket *pkt)
>> +static int storeframe(AVCodecContext *avctx, AVPacket *dst, AVPacket *src)
>> {
>> AOMContext *ctx = avctx->priv_data;
>> - int av_unused pict_type;
>> - int ret = ff_get_encode_buffer(avctx, pkt, cx_frame->sz, 0);
>> - if (ret < 0) {
>> - av_log(avctx, AV_LOG_ERROR,
>> - "Error getting output packet of size %"SIZE_SPECIFIER".\n", cx_frame->sz);
>> - return ret;
>> - }
>> - memcpy(pkt->data, cx_frame->buf, pkt->size);
>> - pkt->pts = pkt->dts = cx_frame->pts;
>> + const uint8_t *sd;
>> + size_t size;
>> + int ret;
>>
>> - if (!!(cx_frame->flags & AOM_FRAME_IS_KEY)) {
>> - pkt->flags |= AV_PKT_FLAG_KEY;
>> -#ifdef AOM_FRAME_IS_INTRAONLY
>> - pict_type = AV_PICTURE_TYPE_I;
>> - } else if (cx_frame->flags & AOM_FRAME_IS_INTRAONLY) {
>> - pict_type = AV_PICTURE_TYPE_I;
>> - } else {
>> - pict_type = AV_PICTURE_TYPE_P;
>> - }
>> -
>> - ff_side_data_set_encoder_stats(pkt, 0, cx_frame->sse + 1,
>> - cx_frame->have_sse ? 3 : 0, pict_type);
>> + av_packet_move_ref(dst, src);
>>
>> - if (cx_frame->have_sse) {
>> + sd = av_packet_get_side_data(dst, AV_PKT_DATA_QUALITY_STATS, &size);
>> + if (sd && size >= 4 + 4 + 8 * 3) {
>> int i;
>> + sd += 4 + 4;
>> for (i = 0; i < 3; ++i) {
>> - avctx->error[i] += cx_frame->sse[i + 1];
>> + avctx->error[i] += bytestream_get_le64(&sd);
>> }
>> - cx_frame->have_sse = 0;
>> -#endif
>> }
>>
>> if (avctx->flags & AV_CODEC_FLAG_GLOBAL_HEADER) {
>> - ret = av_bsf_send_packet(ctx->bsf, pkt);
>> + ret = av_bsf_send_packet(ctx->bsf, dst);
>> if (ret < 0) {
>> av_log(avctx, AV_LOG_ERROR, "extract_extradata filter "
>> "failed to send input packet\n");
>> return ret;
>> }
>> - ret = av_bsf_receive_packet(ctx->bsf, pkt);
>> + ret = av_bsf_receive_packet(ctx->bsf, dst);
>>
>> if (ret < 0) {
>> av_log(avctx, AV_LOG_ERROR, "extract_extradata filter "
>> @@ -1132,7 +1091,7 @@ static int storeframe(AVCodecContext *avctx, struct FrameListData *cx_frame,
>> return ret;
>> }
>> }
>> - return pkt->size;
>> + return dst->size;
>> }
>>
>> /**
>> @@ -1148,16 +1107,14 @@ static int queue_frames(AVCodecContext *avctx, AVPacket *pkt_out)
>> AOMContext *ctx = avctx->priv_data;
>> const struct aom_codec_cx_pkt *pkt;
>> const void *iter = NULL;
>> - int size = 0;
>> + int ret, size = 0;
>>
>> - if (ctx->coded_frame_list) {
>> - struct FrameListData *cx_frame = ctx->coded_frame_list;
>> + if (!avpriv_packet_list_get(&ctx->coded_frame_list, ctx->pkt)) {
>> /* return the leading frame if we've already begun queueing */
>> - size = storeframe(avctx, cx_frame, pkt_out);
>> - if (size < 0)
>> - return size;
>> - ctx->coded_frame_list = cx_frame->next;
>> - free_coded_frame(cx_frame);
>> + ret = storeframe(avctx, pkt_out, ctx->pkt);
>> + if (ret < 0)
>> + goto fail;
>> + size = ret;
>> }
>>
>> /* consume all available output from the encoder before returning. buffers
>> @@ -1165,37 +1122,21 @@ static int queue_frames(AVCodecContext *avctx, AVPacket *pkt_out)
>> while ((pkt = aom_codec_get_cx_data(&ctx->encoder, &iter))) {
>> switch (pkt->kind) {
>> case AOM_CODEC_CX_FRAME_PKT:
>> + ret = cx_pktcpy(avctx, ctx->pkt, pkt);
>> + if (ret < 0)
>> + goto fail;
>> if (!size) {
>> - struct FrameListData cx_frame;
>> -
>> /* avoid storing the frame when the list is empty and we haven't yet
>> * provided a frame for output */
>> - av_assert0(!ctx->coded_frame_list);
>> - cx_pktcpy(ctx, &cx_frame, pkt);
>> - size = storeframe(avctx, &cx_frame, pkt_out);
>> - if (size < 0)
>> - return size;
>> + av_assert0(!ctx->coded_frame_list.head);
>> + ret = storeframe(avctx, pkt_out, ctx->pkt);
>> + if (ret < 0)
>> + goto fail;
>> + size = ret;
>> } else {
>> - struct FrameListData *cx_frame =
>> - av_malloc(sizeof(struct FrameListData));
>> -
>> - if (!cx_frame) {
>> - av_log(avctx, AV_LOG_ERROR,
>> - "Frame queue element alloc failed\n");
>> - return AVERROR(ENOMEM);
>> - }
>> - cx_pktcpy(ctx, cx_frame, pkt);
>> - cx_frame->buf = av_malloc(cx_frame->sz);
>> -
>> - if (!cx_frame->buf) {
>> - av_log(avctx, AV_LOG_ERROR,
>> - "Data buffer alloc (%"SIZE_SPECIFIER" bytes) failed\n",
>> - cx_frame->sz);
>> - av_freep(&cx_frame);
>> - return AVERROR(ENOMEM);
>> - }
>> - memcpy(cx_frame->buf, pkt->data.frame.buf, pkt->data.frame.sz);
>
> I am shocked to see that there were two memcpies.
>
>> - coded_frame_add(&ctx->coded_frame_list, cx_frame);
>> + ret = avpriv_packet_list_put(&ctx->coded_frame_list, ctx->pkt, NULL, 0);
>> + if (ret < 0)
>> + goto fail;
>
> wtf: Any error that queue_frames() returns will be translated to
> "got_packet = 1" by the caller (with return code 0). Error handling in
> this encoder seems to be a joke.
Nice catch. And yeah, it probably should have been reviewed more thoroughly.
>
>> }
>> break;
>> case AOM_CODEC_STATS_PKT:
>> @@ -1236,6 +1177,10 @@ static int queue_frames(AVCodecContext *avctx, AVPacket *pkt_out)
>> }
>>
>> return size;
>> +fail:
>> + av_packet_unref(ctx->pkt);
>> + av_packet_unref(pkt_out);
>> + return ret;
>> }
>>
>> static enum AVPixelFormat aomfmt_to_pixfmt(struct aom_image *img)
>
> _______________________________________________
> 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".
^ permalink raw reply [flat|nested] 4+ messages in thread
* [FFmpeg-devel] [PATCH v2] avcodec/libaomenc: remove one memcpy when queueing packets
2022-08-25 16:29 ` Andreas Rheinhardt
2022-08-25 16:56 ` James Almer
@ 2022-08-25 17:46 ` James Almer
1 sibling, 0 replies; 4+ messages in thread
From: James Almer @ 2022-08-25 17:46 UTC (permalink / raw)
To: ffmpeg-devel
Don't use an intermediary buffer. Achieve this by replacing FrameListData with
a PacketList, and by allocating and populating every packet's payload before
inserting them into the list.
Signed-off-by: James Almer <jamrial@gmail.com>
---
libavcodec/libaomenc.c | 201 +++++++++++++++--------------------------
1 file changed, 74 insertions(+), 127 deletions(-)
diff --git a/libavcodec/libaomenc.c b/libavcodec/libaomenc.c
index a82b933c18..a422e5a4f5 100644
--- a/libavcodec/libaomenc.c
+++ b/libavcodec/libaomenc.c
@@ -38,6 +38,7 @@
#include "av1.h"
#include "avcodec.h"
+#include "bytestream.h"
#include "bsf.h"
#include "codec_internal.h"
#include "encode.h"
@@ -46,24 +47,6 @@
#include "packet_internal.h"
#include "profiles.h"
-/*
- * Portion of struct aom_codec_cx_pkt from aom_encoder.h.
- * One encoded frame returned from the library.
- */
-struct FrameListData {
- void *buf; /**< compressed data buffer */
- size_t sz; /**< length of compressed data */
- int64_t pts; /**< time stamp to show frame
- (in timebase units) */
- unsigned long duration; /**< duration to show frame
- (in timebase units) */
- uint32_t flags; /**< flags for this frame */
- uint64_t sse[4];
- int have_sse; /**< true if we have pending sse[] */
- uint64_t frame_number;
- struct FrameListData *next;
-};
-
typedef struct AOMEncoderContext {
AVClass *class;
AVBSFContext *bsf;
@@ -71,7 +54,8 @@ typedef struct AOMEncoderContext {
struct aom_image rawimg;
struct aom_fixed_buf twopass_stats;
unsigned twopass_stats_size;
- struct FrameListData *coded_frame_list;
+ PacketList coded_frame_list;
+ AVPacket *avpkt;
int cpu_used;
int auto_alt_ref;
int arnr_max_frames;
@@ -283,33 +267,6 @@ static av_cold void dump_enc_cfg(AVCodecContext *avctx,
av_log(avctx, level, "\n");
}
-static void coded_frame_add(void *list, struct FrameListData *cx_frame)
-{
- struct FrameListData **p = list;
-
- while (*p)
- p = &(*p)->next;
- *p = cx_frame;
- cx_frame->next = NULL;
-}
-
-static av_cold void free_coded_frame(struct FrameListData *cx_frame)
-{
- av_freep(&cx_frame->buf);
- av_freep(&cx_frame);
-}
-
-static av_cold void free_frame_list(struct FrameListData *list)
-{
- struct FrameListData *p = list;
-
- while (p) {
- list = list->next;
- free_coded_frame(p);
- p = list;
- }
-}
-
static av_cold int codecctl_int(AVCodecContext *avctx,
#ifdef UENUM1BYTE
aome_enc_control_id id,
@@ -432,7 +389,8 @@ static av_cold int aom_free(AVCodecContext *avctx)
aom_codec_destroy(&ctx->encoder);
av_freep(&ctx->twopass_stats.buf);
av_freep(&avctx->stats_out);
- free_frame_list(ctx->coded_frame_list);
+ avpriv_packet_list_free(&ctx->coded_frame_list);
+ av_packet_free(&ctx->avpkt);
av_bsf_free(&ctx->bsf);
return 0;
}
@@ -1042,6 +1000,10 @@ static av_cold int aom_init(AVCodecContext *avctx,
return ret;
}
+ ctx->avpkt = av_packet_alloc();
+ if (!ctx->avpkt)
+ return AVERROR(ENOMEM);
+
if (enccfg.rc_end_usage == AOM_CBR ||
enccfg.g_pass != AOM_RC_ONE_PASS) {
cpb_props->max_bitrate = avctx->rc_max_rate;
@@ -1053,25 +1015,41 @@ static av_cold int aom_init(AVCodecContext *avctx,
return 0;
}
-static inline void cx_pktcpy(AOMContext *ctx,
- struct FrameListData *dst,
- const struct aom_codec_cx_pkt *src)
+static int cx_pktcpy(AVCodecContext *avctx,
+ AVPacket *dst,
+ const struct aom_codec_cx_pkt *src)
{
- dst->pts = src->data.frame.pts;
- dst->duration = src->data.frame.duration;
- dst->flags = src->data.frame.flags;
- dst->sz = src->data.frame.sz;
- dst->buf = src->data.frame.buf;
+ AOMContext *ctx = avctx->priv_data;
+ int av_unused pict_type;
+ int ret;
+
+ ret = ff_get_encode_buffer(avctx, dst, src->data.frame.sz, 0);
+ if (ret < 0) {
+ av_log(avctx, AV_LOG_ERROR,
+ "Error getting output packet of size %"SIZE_SPECIFIER".\n", src->data.frame.sz);
+ return ret;
+ }
+ memcpy(dst->data, src->data.frame.buf, src->data.frame.sz);
+ dst->pts = dst->dts = src->data.frame.pts;
+
+ if (src->data.frame.flags & AOM_FRAME_IS_KEY) {
+ dst->flags |= AV_PKT_FLAG_KEY;
#ifdef AOM_FRAME_IS_INTRAONLY
- dst->frame_number = ++ctx->frame_number;
- dst->have_sse = ctx->have_sse;
+ pict_type = AV_PICTURE_TYPE_I;
+ } else if (src->data.frame.flags & AOM_FRAME_IS_INTRAONLY) {
+ pict_type = AV_PICTURE_TYPE_I;
+ } else {
+ pict_type = AV_PICTURE_TYPE_P;
+ }
+
if (ctx->have_sse) {
- /* associate last-seen SSE to the frame. */
- /* Transfers ownership from ctx to dst. */
- memcpy(dst->sse, ctx->sse, sizeof(dst->sse));
ctx->have_sse = 0;
- }
+ ret = ff_side_data_set_encoder_stats(dst, 0, ctx->sse + 1, 3, pict_type);
+ if (ret < 0)
+ return ret;
#endif
+ }
+ return 0;
}
/**
@@ -1081,50 +1059,32 @@ static inline void cx_pktcpy(AOMContext *ctx,
* @return packet data size on success
* @return a negative AVERROR on error
*/
-static int storeframe(AVCodecContext *avctx, struct FrameListData *cx_frame,
- AVPacket *pkt)
+static int storeframe(AVCodecContext *avctx, AVPacket *dst, AVPacket *src)
{
AOMContext *ctx = avctx->priv_data;
- int av_unused pict_type;
- int ret = ff_get_encode_buffer(avctx, pkt, cx_frame->sz, 0);
- if (ret < 0) {
- av_log(avctx, AV_LOG_ERROR,
- "Error getting output packet of size %"SIZE_SPECIFIER".\n", cx_frame->sz);
- return ret;
- }
- memcpy(pkt->data, cx_frame->buf, pkt->size);
- pkt->pts = pkt->dts = cx_frame->pts;
-
- if (!!(cx_frame->flags & AOM_FRAME_IS_KEY)) {
- pkt->flags |= AV_PKT_FLAG_KEY;
-#ifdef AOM_FRAME_IS_INTRAONLY
- pict_type = AV_PICTURE_TYPE_I;
- } else if (cx_frame->flags & AOM_FRAME_IS_INTRAONLY) {
- pict_type = AV_PICTURE_TYPE_I;
- } else {
- pict_type = AV_PICTURE_TYPE_P;
- }
+ const uint8_t *sd;
+ size_t size;
+ int ret;
- ff_side_data_set_encoder_stats(pkt, 0, cx_frame->sse + 1,
- cx_frame->have_sse ? 3 : 0, pict_type);
+ av_packet_move_ref(dst, src);
- if (cx_frame->have_sse) {
+ sd = av_packet_get_side_data(dst, AV_PKT_DATA_QUALITY_STATS, &size);
+ if (sd && size >= 4 + 4 + 8 * 3) {
int i;
+ sd += 4 + 4;
for (i = 0; i < 3; ++i) {
- avctx->error[i] += cx_frame->sse[i + 1];
+ avctx->error[i] += bytestream_get_le64(&sd);
}
- cx_frame->have_sse = 0;
-#endif
}
if (avctx->flags & AV_CODEC_FLAG_GLOBAL_HEADER) {
- ret = av_bsf_send_packet(ctx->bsf, pkt);
+ ret = av_bsf_send_packet(ctx->bsf, dst);
if (ret < 0) {
av_log(avctx, AV_LOG_ERROR, "extract_extradata filter "
"failed to send input packet\n");
return ret;
}
- ret = av_bsf_receive_packet(ctx->bsf, pkt);
+ ret = av_bsf_receive_packet(ctx->bsf, dst);
if (ret < 0) {
av_log(avctx, AV_LOG_ERROR, "extract_extradata filter "
@@ -1132,7 +1092,7 @@ static int storeframe(AVCodecContext *avctx, struct FrameListData *cx_frame,
return ret;
}
}
- return pkt->size;
+ return dst->size;
}
/**
@@ -1148,16 +1108,14 @@ static int queue_frames(AVCodecContext *avctx, AVPacket *pkt_out)
AOMContext *ctx = avctx->priv_data;
const struct aom_codec_cx_pkt *pkt;
const void *iter = NULL;
- int size = 0;
+ int ret, size = 0;
- if (ctx->coded_frame_list) {
- struct FrameListData *cx_frame = ctx->coded_frame_list;
+ if (!avpriv_packet_list_get(&ctx->coded_frame_list, ctx->avpkt)) {
/* return the leading frame if we've already begun queueing */
- size = storeframe(avctx, cx_frame, pkt_out);
- if (size < 0)
- return size;
- ctx->coded_frame_list = cx_frame->next;
- free_coded_frame(cx_frame);
+ ret = storeframe(avctx, pkt_out, ctx->avpkt);
+ if (ret < 0)
+ goto fail;
+ size = ret;
}
/* consume all available output from the encoder before returning. buffers
@@ -1165,37 +1123,21 @@ static int queue_frames(AVCodecContext *avctx, AVPacket *pkt_out)
while ((pkt = aom_codec_get_cx_data(&ctx->encoder, &iter))) {
switch (pkt->kind) {
case AOM_CODEC_CX_FRAME_PKT:
+ ret = cx_pktcpy(avctx, ctx->avpkt, pkt);
+ if (ret < 0)
+ goto fail;
if (!size) {
- struct FrameListData cx_frame;
-
/* avoid storing the frame when the list is empty and we haven't yet
* provided a frame for output */
- av_assert0(!ctx->coded_frame_list);
- cx_pktcpy(ctx, &cx_frame, pkt);
- size = storeframe(avctx, &cx_frame, pkt_out);
- if (size < 0)
- return size;
+ av_assert0(!ctx->coded_frame_list.head);
+ ret = storeframe(avctx, pkt_out, ctx->avpkt);
+ if (ret < 0)
+ goto fail;
+ size = ret;
} else {
- struct FrameListData *cx_frame =
- av_malloc(sizeof(struct FrameListData));
-
- if (!cx_frame) {
- av_log(avctx, AV_LOG_ERROR,
- "Frame queue element alloc failed\n");
- return AVERROR(ENOMEM);
- }
- cx_pktcpy(ctx, cx_frame, pkt);
- cx_frame->buf = av_malloc(cx_frame->sz);
-
- if (!cx_frame->buf) {
- av_log(avctx, AV_LOG_ERROR,
- "Data buffer alloc (%"SIZE_SPECIFIER" bytes) failed\n",
- cx_frame->sz);
- av_freep(&cx_frame);
- return AVERROR(ENOMEM);
- }
- memcpy(cx_frame->buf, pkt->data.frame.buf, pkt->data.frame.sz);
- coded_frame_add(&ctx->coded_frame_list, cx_frame);
+ ret = avpriv_packet_list_put(&ctx->coded_frame_list, ctx->avpkt, NULL, 0);
+ if (ret < 0)
+ goto fail;
}
break;
case AOM_CODEC_STATS_PKT:
@@ -1209,7 +1151,8 @@ static int queue_frames(AVCodecContext *avctx, AVPacket *pkt_out)
av_freep(&stats->buf);
stats->sz = 0;
av_log(avctx, AV_LOG_ERROR, "Stat buffer realloc failed\n");
- return AVERROR(ENOMEM);
+ ret = AVERROR(ENOMEM);
+ goto fail;
}
stats->buf = tmp;
memcpy((uint8_t *)stats->buf + stats->sz,
@@ -1236,6 +1179,10 @@ static int queue_frames(AVCodecContext *avctx, AVPacket *pkt_out)
}
return size;
+fail:
+ av_packet_unref(ctx->avpkt);
+ av_packet_unref(pkt_out);
+ return ret;
}
static enum AVPixelFormat aomfmt_to_pixfmt(struct aom_image *img)
--
2.37.2
_______________________________________________
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".
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-08-25 17:47 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-24 22:52 [FFmpeg-devel] [PATCH] avcodec/libaomenc: remove one memcpy when queueing packets James Almer
2022-08-25 16:29 ` Andreas Rheinhardt
2022-08-25 16:56 ` James Almer
2022-08-25 17:46 ` [FFmpeg-devel] [PATCH v2] " James Almer
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
This inbox may be cloned and mirrored by anyone:
git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git
# If you have public-inbox 1.1+ installed, you may
# initialize and index your mirror using the following commands:
public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
ffmpegdev@gitmailbox.com
public-inbox-index ffmpegdev
Example config snippet for mirrors.
AGPL code for this site: git clone https://public-inbox.org/public-inbox.git