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

[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

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

When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- 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/20130811/28bf920d/attachment.asc>