[FFmpeg-devel] [PATCH v2 1/3] avcodec/avpacket: extend AVFrame wrapping in AVPacket

wm4 nfxjfg at gmail.com
Sun Nov 15 12:38:52 CET 2015


On Sun, 15 Nov 2015 15:51:30 +0700
Muhammad Faiz <mfcc64 at gmail.com> wrote:

> From ae6b2c45faac830636602a696925566db03541a2 Mon Sep 17 00:00:00 2001
> From: Muhammad Faiz <mfcc64 at gmail.com>
> Date: Sun, 15 Nov 2015 12:06:12 +0700
> Subject: [PATCH v2 1/3] avcodec/avpacket: extend AVFrame wrapping in AVPacket
> 
> add AV_PKT_FLAG_FRAME
> add av_packet_encode_frame()
> add av_packet_decode_frame()
> add av_packet_get_frame()
> 
> use pointer to AVFrame instead
> properly padded with AV_INPUT_BUFFER_PADDING_SIZE
> 
> modify wrapped_avframe encoder
> implement wrapped_avframe decoder
> implement wrapped_avframe_audio encoder/decoder
> 
> fix avformat/yuv4mpegenc to use av_packet_get_frame()
> ---
>  doc/APIchanges               |  5 +++
>  libavcodec/Makefile          |  3 ++
>  libavcodec/allcodecs.c       |  3 +-
>  libavcodec/avcodec.h         | 29 ++++++++++++++++
>  libavcodec/avpacket.c        | 63 ++++++++++++++++++++++++++++++++++-
>  libavcodec/codec_desc.c      |  7 ++++
>  libavcodec/version.h         |  2 +-
>  libavcodec/wrapped_avframe.c | 78 ++++++++++++++++++++++++++++++++++----------
>  libavformat/yuv4mpegenc.c    |  5 +--
>  9 files changed, 173 insertions(+), 22 deletions(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 14b96ce..9efd44e 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,11 @@ libavutil:     2015-08-28
>  
>  API changes, most recent first:
>  
> +2015-11-15 - lavc 57.16.100 - avcodec.h
> +  Add AV_PKT_FLAG_FRAME.
> +  Add av_packet_encode_frame(), av_packet_decode_frame(),
> +  and av_packet_get_frame().
> +
>  2015-10-29 - lavc 57.12.100 / 57.8.0 - avcodec.h
>    xxxxxx - Deprecate av_free_packet(). Use av_packet_unref() as replacement,
>             it resets the packet in a more consistent way.
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index 68a573f..65d8621 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -577,7 +577,10 @@ OBJS-$(CONFIG_WMV2_ENCODER)            += wmv2enc.o wmv2.o \
>                                            msmpeg4.o msmpeg4enc.o msmpeg4data.o
>  OBJS-$(CONFIG_WNV1_DECODER)            += wnv1.o
>  OBJS-$(CONFIG_WS_SND1_DECODER)         += ws-snd1.o
> +OBJS-$(CONFIG_WRAPPED_AVFRAME_DECODER) += wrapped_avframe.o
>  OBJS-$(CONFIG_WRAPPED_AVFRAME_ENCODER) += wrapped_avframe.o
> +OBJS-$(CONFIG_WRAPPED_AVFRAME_AUDIO_DECODER) += wrapped_avframe.o
> +OBJS-$(CONFIG_WRAPPED_AVFRAME_AUDIO_ENCODER) += wrapped_avframe.o
>  OBJS-$(CONFIG_XAN_DPCM_DECODER)        += dpcm.o
>  OBJS-$(CONFIG_XAN_WC3_DECODER)         += xan.o
>  OBJS-$(CONFIG_XAN_WC4_DECODER)         += xxan.o
> diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
> index 9f60d7c..836fd20 100644
> --- a/libavcodec/allcodecs.c
> +++ b/libavcodec/allcodecs.c
> @@ -342,7 +342,7 @@ void avcodec_register_all(void)
>      REGISTER_DECODER(VP9,               vp9);
>      REGISTER_DECODER(VQA,               vqa);
>      REGISTER_DECODER(WEBP,              webp);
> -    REGISTER_ENCODER(WRAPPED_AVFRAME,   wrapped_avframe);
> +    REGISTER_ENCDEC (WRAPPED_AVFRAME,   wrapped_avframe);
>      REGISTER_ENCDEC (WMV1,              wmv1);
>      REGISTER_ENCDEC (WMV2,              wmv2);
>      REGISTER_DECODER(WMV3,              wmv3);
> @@ -446,6 +446,7 @@ void avcodec_register_all(void)
>      REGISTER_ENCDEC (WMAV1,             wmav1);
>      REGISTER_ENCDEC (WMAV2,             wmav2);
>      REGISTER_DECODER(WMAVOICE,          wmavoice);
> +    REGISTER_ENCDEC (WRAPPED_AVFRAME_AUDIO, wrapped_avframe_audio);
>      REGISTER_DECODER(WS_SND1,           ws_snd1);
>      REGISTER_DECODER(XMA1,              xma1);
>      REGISTER_DECODER(XMA2,              xma2);
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 1af17ed..a318dc5 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -550,6 +550,7 @@ enum AVCodecID {
>                                  * stream (only used by libavformat) */
>      AV_CODEC_ID_FFMETADATA = 0x21000,   ///< Dummy codec for streams containing only metadata information.
>      AV_CODEC_ID_WRAPPED_AVFRAME = 0x21001, ///< Passthrough codec, AVFrames wrapped in AVPacket
> +    AV_CODEC_ID_WRAPPED_AVFRAME_AUDIO = 0x21002,
>  };
>  
>  /**
> @@ -1442,6 +1443,7 @@ typedef struct AVPacket {
>  } AVPacket;
>  #define AV_PKT_FLAG_KEY     0x0001 ///< The packet contains a keyframe
>  #define AV_PKT_FLAG_CORRUPT 0x0002 ///< The packet content is corrupted
> +#define AV_PKT_FLAG_FRAME   0x0004 ///< The packet contains an AVFrame frame

I'd prefer something more "neutral", like a name that indicates that
the packet was constructed from verified data (as opposed to being read
plainly from a file).

>  
>  enum AVSideDataParamChangeFlags {
>      AV_SIDE_DATA_PARAM_CHANGE_CHANNEL_COUNT  = 0x0001,
> @@ -4103,6 +4105,33 @@ int av_packet_copy_props(AVPacket *dst, const AVPacket *src);
>  void av_packet_rescale_ts(AVPacket *pkt, AVRational tb_src, AVRational tb_dst);
>  
>  /**
> + * Encode frame to packet.
> + *
> + * @param pkt destination packet
> + * @param frame source frame
> + * @return 0 on success, a negative AVERROR on failure
> + */
> +int av_packet_encode_frame(AVPacket *pkt, const AVFrame *frame);
> +
> +/**
> + * Decode frame from packet.
> + *
> + * @param pkt source packet
> + * @param frame destination frame
> + * @return 0 on success, a negative AVERROR on failure
> + */
> +int av_packet_decode_frame(const AVPacket *pkt, AVFrame *frame);
> +
> +/**
> + * Get the underlying frame of packet.
> + *
> + * @param pkt packet
> + * @return a pointer to the underlying frame on success,
> + *         NULL when pkt does not contain valid AVFrame
> + */
> +const AVFrame *av_packet_get_frame(const AVPacket *pkt);
> +
> +/**
>   * @}
>   */
>  

This was always a bad hack, and what's even worse, they are bad hacks
for ffmpeg.c (and don't make sense if you look at the API alone). I
agree that it shouldn't grow further by adding public functions for it.

> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> index 1cc10eb..fbb6508 100644
> --- a/libavcodec/avpacket.c
> +++ b/libavcodec/avpacket.c
> @@ -100,7 +100,7 @@ int av_new_packet(AVPacket *pkt, int size)
>  
>  void av_shrink_packet(AVPacket *pkt, int size)
>  {
> -    if (pkt->size <= size)
> +    if (pkt->size <= size || pkt->flags & AV_PKT_FLAG_FRAME)
>          return;

Seems unnecessary. If someone decides to tamper with the packet, it's
already too late.

>      pkt->size = size;
>      memset(pkt->data + size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
> @@ -110,6 +110,10 @@ int av_grow_packet(AVPacket *pkt, int grow_by)
>  {
>      int new_size;
>      av_assert0((unsigned)pkt->size <= INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE);
> +
> +    if (pkt->flags & AV_PKT_FLAG_FRAME)
> +        return AVERROR(EINVAL);
> +

Same here.

>      if (!pkt->size)
>          return av_new_packet(pkt, grow_by);
>      if ((unsigned)grow_by >
> @@ -621,3 +625,60 @@ int ff_side_data_set_encoder_stats(AVPacket *pkt, int quality, int64_t *error, i
>  
>      return 0;
>  }
> +
> +static void packet_frame_release_buffer(void *unused, uint8_t *data)
> +{
> +    av_frame_free((AVFrame **)data);
> +    av_freep(&data);
> +}
> +
> +int av_packet_encode_frame(AVPacket *pkt, const AVFrame *frame)
> +{
> +    AVFrame **data = NULL;
> +    int ret = AVERROR(ENOMEM);
> +
> +    if (!(data = av_mallocz(sizeof(*data) + AV_INPUT_BUFFER_PADDING_SIZE)))
> +        goto fail;
> +
> +    if (!(*data = av_frame_clone(frame)))
> +        goto fail;
> +
> +    /* set all timestamps to frame->pts */
> +    (*data)->pkt_pts = (*data)->pkt_dts = frame->pts;
> +    av_frame_set_best_effort_timestamp(*data, frame->pts);

Let the caller do this.

> +
> +    pkt->buf = av_buffer_create((uint8_t *)data, sizeof(*data) + AV_INPUT_BUFFER_PADDING_SIZE,

Why the buffer padding?

> +                                packet_frame_release_buffer, NULL,
> +                                AV_BUFFER_FLAG_READONLY);
> +    if (!pkt->buf)
> +        goto fail;
> +
> +    pkt->data = pkt->buf->data;
> +    pkt->size = sizeof(*data);
> +    pkt->flags= AV_PKT_FLAG_KEY | AV_PKT_FLAG_FRAME;
> +    pkt->pts = pkt->dts = frame->pts;
> +    pkt->pos  = av_frame_get_pkt_pos(frame);
> +    pkt->duration = av_frame_get_pkt_duration(frame);
> +    return 0;
> +
> +fail:
> +    av_frame_free(data);
> +    av_freep(&data);
> +    return ret;
> +}
> +
> +int av_packet_decode_frame(const AVPacket *pkt, AVFrame *frame)
> +{
> +    if (!(pkt->flags & AV_PKT_FLAG_FRAME) || pkt->data != pkt->buf->data)
> +        return AVERROR(EINVAL);
> +
> +    return av_frame_ref(frame, *(const AVFrame **) pkt->data);
> +}

Maybe as internal API.

> +
> +const AVFrame *av_packet_get_frame(const AVPacket *pkt)
> +{
> +    if (!(pkt->flags & AV_PKT_FLAG_FRAME) || pkt->data != pkt->buf->data)
> +        return NULL;
> +
> +    return *(const AVFrame **) pkt->data;
> +}
> diff --git a/libavcodec/codec_desc.c b/libavcodec/codec_desc.c
> index 9cad3e6..1c63a78 100644
> --- a/libavcodec/codec_desc.c
> +++ b/libavcodec/codec_desc.c
> @@ -2643,6 +2643,13 @@ static const AVCodecDescriptor codec_descriptors[] = {
>          .long_name = NULL_IF_CONFIG_SMALL("Xbox Media Audio 2"),
>          .props     = AV_CODEC_PROP_LOSSY,
>      },
> +    {
> +        .id        = AV_CODEC_ID_WRAPPED_AVFRAME_AUDIO,
> +        .type      = AVMEDIA_TYPE_AUDIO,
> +        .name      = "wrapped_avframe_audio",
> +        .long_name = NULL_IF_CONFIG_SMALL("AVFrame to AVPacket passthrough"),
> +        .props     = AV_CODEC_PROP_LOSSLESS,
> +    },
>  
>      /* subtitle codecs */
>      {
> diff --git a/libavcodec/version.h b/libavcodec/version.h
> index 1e21f15..5eecf5b 100644
> --- a/libavcodec/version.h
> +++ b/libavcodec/version.h
> @@ -29,7 +29,7 @@
>  #include "libavutil/version.h"
>  
>  #define LIBAVCODEC_VERSION_MAJOR  57
> -#define LIBAVCODEC_VERSION_MINOR  15
> +#define LIBAVCODEC_VERSION_MINOR  16
>  #define LIBAVCODEC_VERSION_MICRO 100
>  
>  #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
> diff --git a/libavcodec/wrapped_avframe.c b/libavcodec/wrapped_avframe.c
> index 13c8d8a..981b4d5 100644
> --- a/libavcodec/wrapped_avframe.c
> +++ b/libavcodec/wrapped_avframe.c
> @@ -29,40 +29,57 @@
>  
>  #include "libavutil/internal.h"
>  #include "libavutil/frame.h"
> -#include "libavutil/buffer.h"
>  #include "libavutil/pixdesc.h"
>  
> -static void wrapped_avframe_release_buffer(void *unused, uint8_t *data)
> +static int is_valid_frame(const AVCodecContext *avctx, const AVFrame *frame)
>  {
> -    AVFrame *frame = (AVFrame *)data;
> -
> -    av_frame_free(&frame);
> +    if (frame->format < 0)
> +        return 0;
> +    if (avctx->codec_type == AVMEDIA_TYPE_VIDEO)
> +        return frame->width > 0 && frame->height > 0;
> +    if (avctx->codec_type == AVMEDIA_TYPE_AUDIO)
> +        return frame->nb_samples > 0;
> +    return 0;

Why these extra checks for format/width/height/nb_samples? This barely
makes any sense. You can set a lot of more AVFrame fields to something
invalid.

>  }
>  
>  static int wrapped_avframe_encode(AVCodecContext *avctx, AVPacket *pkt,
>                       const AVFrame *frame, int *got_packet)
>  {
> -    AVFrame *wrapped = av_frame_clone(frame);
> +    int ret;
>  
> -    if (!wrapped)
> -        return AVERROR(ENOMEM);
> +    if (!is_valid_frame(avctx, frame))
> +        return AVERROR(EINVAL);
>  
> -    pkt->buf = av_buffer_create((uint8_t *)wrapped, sizeof(*wrapped),
> -                                wrapped_avframe_release_buffer, NULL,
> -                                AV_BUFFER_FLAG_READONLY);
> -    if (!pkt->buf) {
> -        av_frame_free(&wrapped);
> -        return AVERROR(ENOMEM);
> +    if (pkt->data) {
> +        av_log(avctx, AV_LOG_ERROR, "wrapped_avframe does not support user supplied buffer\n");
> +        return AVERROR(EINVAL);
>      }
>  
> -    pkt->data = (uint8_t *)wrapped;
> -    pkt->size = sizeof(*wrapped);
> +    if ((ret = av_packet_encode_frame(pkt, frame)) < 0)
> +        return ret;
>  
> -    pkt->flags |= AV_PKT_FLAG_KEY;
>      *got_packet = 1;
>      return 0;
>  }
>  
> +static int wrapped_avframe_decode(AVCodecContext *avctx, void *data, int *gotptr,
> +                                  AVPacket *pkt)
> +{
> +    int ret;
> +    AVFrame *out = (AVFrame *) data;
> +    const AVFrame *frame = av_packet_get_frame(pkt);
> +
> +    if (!frame || !is_valid_frame(avctx, frame))
> +        return AVERROR(EINVAL);
> +
> +    if ((ret = av_packet_decode_frame(pkt, out)) < 0)
> +        return ret;
> +
> +    *gotptr = 1;
> +
> +    return pkt->size;
> +}
> +
>  AVCodec ff_wrapped_avframe_encoder = {
>      .name           = "wrapped_avframe",
>      .long_name      = NULL_IF_CONFIG_SMALL("AVFrame to AVPacket passthrough"),
> @@ -71,3 +88,30 @@ AVCodec ff_wrapped_avframe_encoder = {
>      .encode2        = wrapped_avframe_encode,
>      .caps_internal  = FF_CODEC_CAP_INIT_THREADSAFE,
>  };
> +
> +AVCodec ff_wrapped_avframe_audio_encoder = {
> +    .name           = "wrapped_avframe_audio",
> +    .long_name      = NULL_IF_CONFIG_SMALL("AVFrame to AVPacket passthrough"),
> +    .type           = AVMEDIA_TYPE_AUDIO,
> +    .id             = AV_CODEC_ID_WRAPPED_AVFRAME_AUDIO,
> +    .encode2        = wrapped_avframe_encode,
> +    .caps_internal  = FF_CODEC_CAP_INIT_THREADSAFE,
> +};
> +
> +AVCodec ff_wrapped_avframe_decoder = {
> +    .name           = "wrapped_avframe",
> +    .long_name      = NULL_IF_CONFIG_SMALL("AVFrame to AVPacket passthrough"),
> +    .type           = AVMEDIA_TYPE_VIDEO,
> +    .id             = AV_CODEC_ID_WRAPPED_AVFRAME,
> +    .decode         = wrapped_avframe_decode,
> +    .caps_internal  = FF_CODEC_CAP_INIT_THREADSAFE,
> +};
> +
> +AVCodec ff_wrapped_avframe_audio_decoder = {
> +    .name           = "wrapped_avframe_audio",
> +    .long_name      = NULL_IF_CONFIG_SMALL("AVFrame to AVPacket passthrough"),
> +    .type           = AVMEDIA_TYPE_AUDIO,
> +    .id             = AV_CODEC_ID_WRAPPED_AVFRAME_AUDIO,
> +    .decode         = wrapped_avframe_decode,
> +    .caps_internal  = FF_CODEC_CAP_INIT_THREADSAFE,
> +};
> diff --git a/libavformat/yuv4mpegenc.c b/libavformat/yuv4mpegenc.c
> index 25bf13f..3683f1a 100644
> --- a/libavformat/yuv4mpegenc.c
> +++ b/libavformat/yuv4mpegenc.c
> @@ -138,14 +138,15 @@ static int yuv4_write_packet(AVFormatContext *s, AVPacket *pkt)
>  {
>      AVStream *st = s->streams[pkt->stream_index];
>      AVIOContext *pb = s->pb;
> -    AVFrame *frame;
> +    const AVFrame *frame;

Why this change? const doesn't really work in C anyway.

>      int* first_pkt = s->priv_data;
>      int width, height, h_chroma_shift, v_chroma_shift;
>      int i;
>      char buf2[Y4M_LINE_MAX + 1];
>      uint8_t *ptr, *ptr1, *ptr2;
>  
> -    frame = (AVFrame *)pkt->data;
> +    if (!(frame = av_packet_get_frame(pkt)))
> +        return AVERROR(EINVAL);
>  
>      /* for the first packet we have to output the header as well */
>      if (*first_pkt) {



More information about the ffmpeg-devel mailing list