[FFmpeg-devel] [PATCH 1/2] avcodec/wrapped_avframe: implement wrapped_avframe decoder

Muhammad Faiz mfcc64 at gmail.com
Fri Nov 13 18:57:02 CET 2015


On Wed, Nov 11, 2015 at 4:53 AM, wm4 <nfxjfg at googlemail.com> wrote:
> On Tue, 10 Nov 2015 22:42:31 +0100
> Michael Niedermayer <michael at niedermayer.cc> wrote:
>
>> On Tue, Nov 10, 2015 at 03:15:50PM +0100, wm4 wrote:
>> > On Tue, 10 Nov 2015 14:31:26 +0100
>> > Michael Niedermayer <michael at niedermayer.cc> wrote:
>> >
>> > > On Tue, Nov 10, 2015 at 04:54:17PM +0700, Muhammad Faiz wrote:
>> > > > On Mon, Nov 9, 2015 at 11:22 PM, wm4 <nfxjfg at googlemail.com> wrote:
>> > > > > On Mon, 9 Nov 2015 08:03:54 -0800
>> > > > > Muhammad Faiz <mfcc64 at gmail.com> wrote:
>> > > > >
>> > > > >> From 4dcbda2e585404d2d79d5afcdc13fcb699f6f158 Mon Sep 17 00:00:00 2001
>> > > > >> From: Muhammad Faiz <mfcc64 at gmail.com>
>> > > > >> Date: Mon, 9 Nov 2015 15:55:13 +0700
>> > > > >> Subject: [PATCH 1/2] avcodec/wrapped_avframe: implement wrapped_avframe
>> > > > >>  decoder
>> > > > >>
>> > > > >> fix ticket #4985
>> > > > >> for use in avdevice/lavfi
>> > > > >> ---
>> > > > >>  libavcodec/Makefile          |  1 +
>> > > > >>  libavcodec/allcodecs.c       |  2 +-
>> > > > >>  libavcodec/version.h         |  2 +-
>> > > > >>  libavcodec/wrapped_avframe.c | 32 ++++++++++++++++++++++++++++++++
>> > > > >>  4 files changed, 35 insertions(+), 2 deletions(-)
>> > > > >>
>> > > > >> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
>> > > > >> index 68a573f..c60d512 100644
>> > > > >> --- a/libavcodec/Makefile
>> > > > >> +++ b/libavcodec/Makefile
>> > > > >> @@ -577,6 +577,7 @@ 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_XAN_DPCM_DECODER)        += dpcm.o
>> > > > >>  OBJS-$(CONFIG_XAN_WC3_DECODER)         += xan.o
>> > > > >> diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
>> > > > >> index 9f60d7c..3260927 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);
>> > > > >> 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..185a1a2 100644
>> > > > >> --- a/libavcodec/wrapped_avframe.c
>> > > > >> +++ b/libavcodec/wrapped_avframe.c
>> > > > >> @@ -32,6 +32,8 @@
>> > > > >>  #include "libavutil/buffer.h"
>> > > > >>  #include "libavutil/pixdesc.h"
>> > > > >>
>> > > > >> +#if CONFIG_WRAPPED_AVFRAME_ENCODER
>> > > > >> +
>> > > > >>  static void wrapped_avframe_release_buffer(void *unused, uint8_t *data)
>> > > > >>  {
>> > > > >>      AVFrame *frame = (AVFrame *)data;
>> > > > >> @@ -71,3 +73,33 @@ AVCodec ff_wrapped_avframe_encoder = {
>> > > > >>      .encode2        = wrapped_avframe_encode,
>> > > > >>      .caps_internal  = FF_CODEC_CAP_INIT_THREADSAFE,
>> > > > >>  };
>> > > > >> +
>> > > > >> +#endif
>> > > > >> +
>> > > > >> +#if CONFIG_WRAPPED_AVFRAME_DECODER
>> > > > >> +
>> > > > >> +static int wrapped_avframe_decode(AVCodecContext *avctx, void *data,
>> > > > >> +                                  int *got_frame, AVPacket *avpkt)
>> > > > >> +{
>> > > > >> +    int ret;
>> > > > >> +
>> > > > >> +    if (avpkt->size != sizeof(AVFrame))
>> > > > >> +        return AVERROR(EINVAL);
>> > > > >> +
>> > > > >> +    if ((ret = av_frame_ref((AVFrame *) data, (AVFrame *) avpkt->data)) < 0)
>> > > > >> +        return ret;
>> > > > >> +
>> > > > >> +    *got_frame = 1;
>> > > > >> +    return avpkt->size;
>> > > > >> +}
>> > > > >> +
>> > > > >> +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,
>> > > > >> +};
>> > > > >> +
>> > > > >> +#endif
>> > > > >
>> > > > > This is very dangerous. You get potentially security relevant bad
>> > > > > behavior if you get anyone to force a demuxer/decoder on an untrusted
>> > > > > input file.
>> > > >
>> > > > OK, this makes segfault on my machine (120 = sizeof(AVFrame)/4 on my machine)
>> > > > ffmpeg -codec wrapped_avframe  -pixel_format rgba -f rawvideo -s 120x1
>> > > > -i input.mkv -f null -y /dev/null
>> > >
>> > > maybe in addition to any other saftey features
>> > > "-codec wrapped_avframe" could be checked for and forbidden in
>> > > libavformat, or does this have any safe use ?
>> >
>> > You could argue that API users can expect that there will be no
>> > security or stability issues even if they let an user decide with which
>> > codec to initialize a AVCodecContext.
>> >
>> > I'm unsure to what extent we want to fulfill such expectations.
>>
>> it would be ideal if it was safe for the user to set any codec
>>
>> about the packet flags idea, that potentially might break user apps
>> which do not pass unknown new flags between their demuxer and
>> decoder layers.
>> So if we do that, we must document it in APIChanges or similar
>>
>> dont really know what is best
>
> A side data could possibly be used to signal this as well.
>
> Anyway, surely someone has a better idea.

Packet flags is better and simpler if we ignore compatibility issue,
side data is OK though more complicated,
maybe it should be encapsulated using API (av_packet_frame_*)
to hide the implementation and remove duplicate code.

Not sure what should I do, but anyway thank's for the review


More information about the ffmpeg-devel mailing list