LibTiff Mailing List
TIFF and LibTiff Mailing List Archive
February 2009
Previous Thread
Next Thread
Previous by Thread
Next by Thread
Previous by Date
Next by Date
Contact
The TIFF Mailing List Homepage
This list is run by Frank Warmerdam
Archive maintained by AWare Systems
Thread
2009.02.07 10:53 "Re: assertions, and building with DEBUG/NDEBUG", by Joris Van Damme
Frank,
> I would appreciate it if you could get a bugzilla account, and be
> prepared to interact with bugs there. In the meantime I've appended
> your email to the ticket.
OK.
> In TIFFFetchNormalTag() we have lots of logic like:
>
> case TIFF_SETGET_UINT16:
> {
> uint16 data;
> assert(fip->field_readcount==1);
> assert(fip->field_passcount==0);
> err=TIFFReadDirEntryShort(tif,dp,&data);
> if (err==TIFFReadDirEntryErrOk)
> {
> if (!TIFFSetField(tif,dp->tdir_tag,data))
> return(0);
> }
> }
> break;
>
> Would it be better to change these assertions to a runtime error
> indicating that this tag is not suitable for processing in
> TIFFFetchNormalTag(), possibly indicating a corrupt file or
> a logic error in libtiff?
Personaly, I don't think that's a good idea. If you have encapsulating logic
working one way (the different stages in TIFFReadDirectory), and you have
called logic underneath, then the latter shouldn't normally be build to
withstand the encapsulating logic failing. Rather, the called logic should
be build to raise our LibTiff developers interest if/when the calling logic
fails, so that we know exactly what is going on and why, and can assess the
actual problem rather then obscure its symptoms.
> This is quite a dirty file so I'm pleased to settle with reasonable
> error reports and recovery.
>
> I presume the "fip" description of the tag cannot be made compatible
> with TIFFFetchNormalTag for some reason? It does not *seem* like a
> particularly complicated tag to handle generically. It is currently:
>
> { TIFFTAG_COMPRESSION, -1, 1, TIFF_SHORT, 0, TIFF_SETGET_UINT16,
> TIFF_SETGET_UNDEFINED, FIELD_COMPRESSION, 0, 0, "Compression", NULL },
>
> I a bit lost in aspects of this - particularly the stuff about
> set_field_type and get_field_type in the TIFFField structure.
I think we're all convinced of the trouble with the varargs calling protocol
in TIFFGetField and TIFFSetField. The set_field_type and get_field_type is
best regarded a first attempt at making the library 'aware' on a meta-level
of what types are expected in the TIFFGetField and TIFFSetField interface
for the various tags. This is required for a) moving away from specific
handling, towards more generic handling, in some of the lowest level under
the hood code that is the deepest implementation of TIFFGetField and
TIFFSetField, and b) it is also required to one day build a backwards
compatible TIFFGetField and TIFFSetField that calls a newer more robust and
generic tag interface. Both a) and b) are mostly long-term goals at best,
and it's anybody's guess whether they'll ever be a reality, but jumping to
the major update 4.0 seemed like a good time to get at least some primary
requirements started.
That's not to say it's the only use for these, that's not to say it's
complete or perfect in reaching its goals, and I'm not saying it's all
purely long-term anticipating either - in fact some of a) is already in
place if I remember correctly. What I am saying is that it might be most
productive if we think of these this way and try to take it from there.
> Actually, can't we just set the field_readcount to 1 and then this
> definition would work?
I understand, and it seems like an option. Furthermore, I don't wish to
defend the crappy logic in TIFFReadDirectory, as I think it's simply not
good.
But, short of rewriting LibTiff, building on a proper generic tag and IFD
handling layer, subsequently adding a full lazy as-needed
image-interpretation layer, TIFFReadDirectory logic stands, and we best
ensure it works.
So, I still feel you're obscuring one symptom with the cure you propose
here, where you might easilly choose to deal with the problem instead. If
you do as you propose here, you'll get the exact same problem again if a
file with multiple TIFFTAG_SAMPLESPERPIXEL tags in an IFD surfaces sooner or
later, and there is certainly also potential for other, more obscure and
much harder to diagnose trouble because of the fix-up hacks in
TIFFReadDirectory that are bound to particular stages and get into trouble
if some of their prerequisits gets overwritten in a subsequent stage. What
happens if a second TIFFTAG_COMPRESSION tag has a legit datatype and value,
different from the first, and overwrites the value in the third
TIFFReadDirectory stage? Does it depend on the exact values? Might it depend
on other specific compression-type dependent hacks scattered all over the
library? Would you like to guess what happens to multiple
TIFFTAG_YCBCRSUBSAMPLING tags combined with a subsampling-autocorrecting
compression mode like JPEG? What if they've different values? Even
investigating these situations and estimating the problems seems like a lot
of unpleasant work.
In conclusion, we've a crappy TIFFReadDirectory. Due to its multi-stage
design handling a lot of things in a very specific manner, it's fragile and
complicated. It much depends on the IGNORE mechanism to avoid reading any
given data twice. It gets into trouble if the IGNORE mechanism fails due to
a given tag being present twice. To avoid even more complicated situations,
I propose we make sure the IGNORE mechanism works even if there are multiple
instances of a tag, but first ensuring there aren't, sorting the tags making
all unique at the start of TIFFReadDirectory, rather then just dig a hole to
put this particular symptom with this particular file in and having this
opportunity for a clear and easy but profound fix go to waste whilst adding
to the complexity of the situation.
Best regards,
Joris