[FFmpeg-devel] [Read EXIF metadata 1/3] Refactor TIFF tag related functions to share the code.

[Thilo Borgmann]

Am 11.08.13 14:26, schrieb Michael Niedermayer:
> On Sat, Aug 10, 2013 at 10:35:29PM +0200, Thilo Borgmann wrote:
>> Am 10.08.13 22:34, schrieb Thilo Borgmann:
>>> 1/3 rev 6
>>>
>>> -Thilo
>>>
>>
> 
> [...]
> 
>> +int ff_tread_tag(GetByteContext *gb, int le, unsigned *tag, unsigned *type,
>> +                 unsigned *count, int *next)
>> +{
>> +    int ifd_tag;
>> +    int valid_type;
>> +
>> +    *tag    = ff_tget_short(gb, le);
>> +    *type   = ff_tget_short(gb, le);
>> +    *count  = ff_tget_long (gb, le);
>> +
>> +    ifd_tag    = ff_tis_ifd(*tag);
>> +//    valid_type = ifd_tag || (*type != 0 && *type < FF_ARRAY_ELEMS(type_sizes));
>> +    valid_type = *type != 0 && *type < FF_ARRAY_ELEMS(type_sizes);
>> +
>> +    *next = bytestream2_tell(gb) + 4;
>> +
>> +    // check for valid type
>> +    if (!valid_type) {
>> +        return AVERROR_INVALIDDATA;
>> +    }
>> +
>> +    // seek to offset if this is an IFD-tag or
>> +    // if count values do not fit into the offset value
>> +    if (ifd_tag || (*count > 4 || !(type_sizes[*type] * (*count) <= 4 || *type == TIFF_STRING))) {
>> +        bytestream2_seek(gb, ff_tget_long (gb, le), SEEK_SET);
>> +    }
> 
> I think the seek is somewhat unexpected here
> 
> I mean as it is one needs
> ff_tread_tag()
> 
> on error
>     seek back
> 
> on success
>     seek back
> 
> 
> I think it would be much cleaner and more symmetrical if it was
> ff_tread_tag()
> seek forward
> 
> on error
>     seek back
> 
> on success
>     seek back

I don't agree... for tiff and exif it is:

ff_tread_tag()

on success
     read value (because the value data now begins at the next byte)

seek to next tag (from wherever, maybe seek to current position)


IIRC in a prior you said you would like to have all the reading and seeking in
one function if possible - that's what it could be like.

-Thilo