On Sun, 15 Nov 2015 15:51:30 +0700
Muhammad Faiz <mfc...@gmail.com> wrote:
> From ae6b2c45faac830636602a696925566db03541a2 Mon Sep 17 00:00:00 2001
> From: Muhammad Faiz <mfc...@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) {
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
