[FFmpeg-devel] [PATCH] tiff: fix handling of metadata with refcounted frames

[Michael Niedermayer]

On Wed, Mar 13, 2013 at 08:06:57PM +0100, Hendrik Leppkes wrote:
> On Wed, Mar 13, 2013 at 7:48 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Wed, Mar 13, 2013 at 06:51:17PM +0100, Hendrik Leppkes wrote:
> >> Since the conversion to refcounted frames, the tiff decoder
> >> only wrote the metadata into its internal "picture" in its own context,
> >> never exposing the metadata to the user, and eventually leaking the
> >> metadata.
> >>
> >> Instead, properly store the metadata directly into the frame being decoded into.
> >> ---
> >>  libavcodec/tiff.c | 32 ++++++++++++++------------------
> >>  1 file changed, 14 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c
> >> index b749a6b..70f4f32 100644
> >> --- a/libavcodec/tiff.c
> >> +++ b/libavcodec/tiff.c
> >> @@ -44,7 +44,6 @@
> >>  typedef struct TiffContext {
> >>      AVCodecContext *avctx;
> >>      GetByteContext gb;
> >> -    AVFrame picture;
> >>
> >>      int width, height;
> >>      unsigned int bpp, bppcount;
> >> @@ -268,7 +267,7 @@ static char *shorts2str(int16_t *sp, int count, const char *sep)
> >>
> >>  static int add_doubles_metadata(int count,
> >>                                  const char *name, const char *sep,
> >> -                                TiffContext *s)
> >> +                                TiffContext *s, AVFrame *frame)
> >>  {
> >>      char *ap;
> >>      int i;
> >> @@ -289,12 +288,12 @@ static int add_doubles_metadata(int count,
> >>      av_freep(&dp);
> >>      if (!ap)
> >>          return AVERROR(ENOMEM);
> >> -    av_dict_set(avpriv_frame_get_metadatap(&s->picture), name, ap, AV_DICT_DONT_STRDUP_VAL);
> >> +    av_dict_set(avpriv_frame_get_metadatap(frame), name, ap, AV_DICT_DONT_STRDUP_VAL);
> >>      return 0;
> >>  }
> >>
> >>  static int add_shorts_metadata(int count, const char *name,
> >> -                               const char *sep, TiffContext *s)
> >> +                               const char *sep, TiffContext *s, AVFrame *frame)
> >>  {
> >>      char *ap;
> >>      int i;
> >> @@ -315,12 +314,12 @@ static int add_shorts_metadata(int count, const char *name,
> >>      av_freep(&sp);
> >>      if (!ap)
> >>          return AVERROR(ENOMEM);
> >> -    av_dict_set(avpriv_frame_get_metadatap(&s->picture), name, ap, AV_DICT_DONT_STRDUP_VAL);
> >> +    av_dict_set(avpriv_frame_get_metadatap(frame), name, ap, AV_DICT_DONT_STRDUP_VAL);
> >>      return 0;
> >>  }
> >>
> >>  static int add_string_metadata(int count, const char *name,
> >> -                               TiffContext *s)
> >> +                               TiffContext *s, AVFrame *frame)
> >>  {
> >>      char *value;
> >>
> >> @@ -334,17 +333,17 @@ static int add_string_metadata(int count, const char *name,
> >>      bytestream2_get_bufferu(&s->gb, value, count);
> >>      value[count] = 0;
> >>
> >> -    av_dict_set(avpriv_frame_get_metadatap(&s->picture), name, value, AV_DICT_DONT_STRDUP_VAL);
> >> +    av_dict_set(avpriv_frame_get_metadatap(frame), name, value, AV_DICT_DONT_STRDUP_VAL);
> >>      return 0;
> >>  }
> >>
> >>  static int add_metadata(int count, int type,
> >> -                        const char *name, const char *sep, TiffContext *s)
> >> +                        const char *name, const char *sep, TiffContext *s, AVFrame *frame)
> >>  {
> >>      switch(type) {
> >> -    case TIFF_DOUBLE: return add_doubles_metadata(count, name, sep, s);
> >> -    case TIFF_SHORT : return add_shorts_metadata(count, name, sep, s);
> >> -    case TIFF_STRING: return add_string_metadata(count, name, s);
> >> +    case TIFF_DOUBLE: return add_doubles_metadata(count, name, sep, s, frame);
> >> +    case TIFF_SHORT : return add_shorts_metadata(count, name, sep, s, frame);
> >> +    case TIFF_STRING: return add_string_metadata(count, name, s, frame);
> >>      default         : return AVERROR_INVALIDDATA;
> >>      };
> >>  }
> >
> > wouldnt it be simpler to put the pointer to the frame in the context
> > instead of passing it intoi so many functions ?
> >
> >
> 
> I can do it that way if you prefer, but personally i prefer this
> method, and keeping the context to actual persistent data.

ok, applied

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Good people do not need laws to tell them to act responsibly, while bad
people will find a way around the laws. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130313/a1991662/attachment.asc>