[FFmpeg-devel] [PATCH v2 1/3] avcodec/avpacket: extend AVFrame wrapping in AVPacket
Muhammad Faiz
mfcc64 at gmail.com
Sun Nov 15 18:50:24 CET 2015
On Sun, Nov 15, 2015 at 6:38 PM, wm4 <nfxjfg at gmail.com> wrote:
> 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).
probably AV_PKT_FLAG_WRAPPED_AVFRAME
>
>>
>> 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.
API is for hiding the implementation,
without API, it is OK. If all code are consistent (e.g. in yuv4mpegenc, lavfi)
>
>> 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.
OK
>
>> 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?
Is it OK without buffer padding?
If it is, previous implementation is OK.
>
>> + 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.
without API is OK actually when all code are consistent.
>
>> +
>> +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.
OK, maybe it is not necessary
>
>> }
>>
>> 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.
Why doesn't work? It is similar to const char *.
Ok, but actually it is unnecessary.
>
>> 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) {
>
Thank's.
More information about the ffmpeg-devel
mailing list