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

[Hendrik Leppkes]

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.

- Hendrik