[FFmpeg-devel] [PATCH] libavcodec/libaomenc.c: Added code for computing PSNR/SSIM for libaom encoder.
Sam John
samjohn at google.com
Fri Sep 14 23:53:53 EEST 2018
James,
> Did you copy this chunk from the libvpxenc wrapper? Because I don't
>think this is valid at all for libaom. A quick grep on their tree shows
>that AOM_FRAME_IS_INVISIBLE is never set anywhere. It looks like a
>leftover flag they forgot to remove from the libvpx codebase
I used the same logic as libvpx. The flag AOM_FRAME_IS_INVISIBLE is not set
at present. But I hope that this flag will be updated soon. I will update
the code to remove this flag from my patch for now.
> Does libaom have a flag or field to signal the type of the visible frame
> in the returned packet?
At present libaom doesn't set the flag for the INTRA_ONLY in the returned
packet. Until this flag is updated, we can use the same logic as libvpx.
I will make these corrections and update the patch for review.
On Sat, Sep 8, 2018 at 12:23 PM James Almer <jamrial at gmail.com> wrote:
> On 9/7/2018 8:51 PM, Sam John wrote:
> > ---
> > libavcodec/libaomenc.c | 117 +++++++++++++++++++++++++++++++++--------
> > 1 file changed, 96 insertions(+), 21 deletions(-)
> >
> > diff --git a/libavcodec/libaomenc.c b/libavcodec/libaomenc.c
> > index 9431179886..e62057177d 100644
> > --- a/libavcodec/libaomenc.c
> > +++ b/libavcodec/libaomenc.c
> > @@ -50,6 +50,9 @@ struct FrameListData {
> > 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;
> > };
> >
> > @@ -68,6 +71,9 @@ typedef struct AOMEncoderContext {
> > int static_thresh;
> > int drop_threshold;
> > int noise_sensitivity;
> > + uint64_t sse[4];
> > + int have_sse; /**< true if we have pending sse[] */
> > + uint64_t frame_number;
> > } AOMContext;
> >
> > static const char *const ctlidstr[] = {
> > @@ -289,7 +295,8 @@ static av_cold int aom_init(AVCodecContext *avctx,
> > {
> > AOMContext *ctx = avctx->priv_data;
> > struct aom_codec_enc_cfg enccfg = { 0 };
> > - aom_codec_flags_t flags = 0;
> > + aom_codec_flags_t flags =
> > + (avctx->flags & AV_CODEC_FLAG_PSNR) ? AOM_CODEC_USE_PSNR : 0;
> > AVCPBProperties *cpb_props;
> > int res;
> > aom_img_fmt_t img_fmt;
> > @@ -499,13 +506,30 @@ static av_cold int aom_init(AVCodecContext *avctx,
> > }
> >
> > static inline void cx_pktcpy(struct FrameListData *dst,
> > - const struct aom_codec_cx_pkt *src)
> > + const struct aom_codec_cx_pkt *src,
> > + AOMContext *ctx)
> > {
> > 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;
> > + dst->have_sse = 0;
> > + /* For alt-ref frame, don't store PSNR or increment frame_number */
> > + if (!(dst->flags & AOM_FRAME_IS_INVISIBLE)) {
>
> Did you copy this chunk from the libvpxenc wrapper? Because I don't
> think this is valid at all for libaom. A quick grep on their tree shows
> that AOM_FRAME_IS_INVISIBLE is never set anywhere. It looks like a
> leftover flag they forgot to remove from the libvpx codebase.
>
> > + dst->frame_number = ++ctx->frame_number;
> > + dst->have_sse = ctx->have_sse;
> > + if (ctx->have_sse) {
> > + /* associate last-seen SSE to the frame. */
> > + /* Transfers ownership from ctx to dst. */
> > + /* WARNING! This makes the assumption that PSNR_PKT comes
> > + just before the frame it refers to! */
> > + memcpy(dst->sse, ctx->sse, sizeof(dst->sse));
> > + ctx->have_sse = 0;
> > + }
> > + } else {
> > + dst->frame_number = -1; /* sanity marker */
> > + }
> > }
> >
> > /**
> > @@ -524,26 +548,68 @@ static int storeframe(AVCodecContext *avctx,
> struct FrameListData *cx_frame,
> > 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;
> > + } else {
> > + int pict_type;
> > + memcpy(pkt->data, cx_frame->buf, pkt->size);
> > + pkt->pts = pkt->dts = cx_frame->pts;
> > +#if FF_API_CODED_FRAME
> > +FF_DISABLE_DEPRECATION_WARNINGS
> > + avctx->coded_frame->pts = cx_frame->pts;
> > + avctx->coded_frame->key_frame = !!(cx_frame->flags &
> AOM_FRAME_IS_KEY);
>
> coded_frame is deprecated and it's not meant to be used in new modules.
> It's left on old ones until removal for backwards compatibility reasons.
>
> > +FF_ENABLE_DEPRECATION_WARNINGS
> > +#endif
> > +
> > + if (!!(cx_frame->flags & AOM_FRAME_IS_KEY)) {
> > + pict_type = AV_PICTURE_TYPE_I;
> > +#if FF_API_CODED_FRAME
> > +FF_DISABLE_DEPRECATION_WARNINGS
> > + avctx->coded_frame->pict_type = pict_type;
> > +FF_ENABLE_DEPRECATION_WARNINGS
> > +#endif
> > + pkt->flags |= AV_PKT_FLAG_KEY;
> > + } else {
> > + pict_type = AV_PICTURE_TYPE_P;
>
> I'm fairly sure libaom can return I frames that are not key frames. The
> INTRA_ONLY type ones.
>
> Does libaom have a flag or field to signal the type of the visible frame
> in the returned packet?
>
> > +#if FF_API_CODED_FRAME
> > +FF_DISABLE_DEPRECATION_WARNINGS
> > + avctx->coded_frame->pict_type = pict_type;
> > +FF_ENABLE_DEPRECATION_WARNINGS
> > +#endif
> > + }
> >
> > - if (!!(cx_frame->flags & AOM_FRAME_IS_KEY))
> > - pkt->flags |= AV_PKT_FLAG_KEY;
> > + if (avctx->flags & AV_CODEC_FLAG_GLOBAL_HEADER) {
> > + ret = av_bsf_send_packet(ctx->bsf, pkt);
> > + 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);
> >
> > - if (avctx->flags & AV_CODEC_FLAG_GLOBAL_HEADER) {
> > - ret = av_bsf_send_packet(ctx->bsf, pkt);
> > - if (ret < 0) {
> > - av_log(avctx, AV_LOG_ERROR, "extract_extradata filter "
> > - "failed to send input packet\n");
> > - return ret;
> > + if (ret < 0) {
> > + av_log(avctx, AV_LOG_ERROR, "extract_extradata filter "
> > + "failed to receive output packet\n");
> > + return ret;
> > + }
> > }
> > - ret = av_bsf_receive_packet(ctx->bsf, pkt);
> >
> > - if (ret < 0) {
> > - av_log(avctx, AV_LOG_ERROR, "extract_extradata filter "
> > - "failed to receive output packet\n");
> > - return ret;
>
> Don't include reindentation when it's a lot of code. It makes the patch
> harder to read.
>
> > + ff_side_data_set_encoder_stats(pkt, 0, cx_frame->sse + 1,
> > + cx_frame->have_sse ? 3 : 0,
> pict_type);
> > +
> > + if (cx_frame->have_sse) {
> > + int i;
> > + /* Beware of the Y/U/V/all order! */
> > +#if FF_API_CODED_FRAME
> > +FF_DISABLE_DEPRECATION_WARNINGS
> > + avctx->coded_frame->error[0] = cx_frame->sse[1];
> > + avctx->coded_frame->error[1] = cx_frame->sse[2];
> > + avctx->coded_frame->error[2] = cx_frame->sse[3];
> > + avctx->coded_frame->error[3] = 0; // alpha
> > +FF_ENABLE_DEPRECATION_WARNINGS
> > +#endif
> > + for (i = 0; i < 3; ++i) {
> > + avctx->error[i] += cx_frame->sse[i + 1];
> > + }
> > + cx_frame->have_sse = 0;
> > }
> > }
> > return pkt->size;
> > @@ -585,7 +651,7 @@ static int queue_frames(AVCodecContext *avctx,
> AVPacket *pkt_out)
> > /* 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(&cx_frame, pkt);
> > + cx_pktcpy(&cx_frame, pkt, ctx);
> > size = storeframe(avctx, &cx_frame, pkt_out);
> > if (size < 0)
> > return size;
> > @@ -598,7 +664,7 @@ static int queue_frames(AVCodecContext *avctx,
> AVPacket *pkt_out)
> > "Frame queue element alloc failed\n");
> > return AVERROR(ENOMEM);
> > }
> > - cx_pktcpy(cx_frame, pkt);
> > + cx_pktcpy(cx_frame, pkt, ctx);
> > cx_frame->buf = av_malloc(cx_frame->sz);
> >
> > if (!cx_frame->buf) {
> > @@ -628,7 +694,16 @@ static int queue_frames(AVCodecContext *avctx,
> AVPacket *pkt_out)
> > stats->sz += pkt->data.twopass_stats.sz;
> > break;
> > }
> > - case AOM_CODEC_PSNR_PKT: // FIXME add support for
> AV_CODEC_FLAG_PSNR
> > + case AOM_CODEC_PSNR_PKT:
> > + {
> > + av_assert0(!ctx->have_sse);
> > + ctx->sse[0] = pkt->data.psnr.sse[0];
> > + ctx->sse[1] = pkt->data.psnr.sse[1];
> > + ctx->sse[2] = pkt->data.psnr.sse[2];
> > + ctx->sse[3] = pkt->data.psnr.sse[3];
> > + ctx->have_sse = 1;
> > + break;
> > + }
> > case AOM_CODEC_CUSTOM_PKT:
> > // ignore unsupported/unrecognized packet types
> > break;
> >
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
More information about the ffmpeg-devel
mailing list