> Seems good to me. I would suggest sending a patch upstream, see what
> they think.
Thanks for the feedback ! I'll write the remaining part and submit it to
upstream.
> Also I tend to think some some of assertion might be a good idea,
> something that aborts if
>
> (td->td_samplesperpixel - td->td_extrasamples) > 3
>
> As aborting early is probably better then screwing up memory.
>
> For reference, the next value in the struct is:
>
> typedef struct {
> [...]
>
> /* Colorimetry parameters */
> uint16* td_transferfunction[3];
> float* td_refblackwhite;
>
> [...]
> } TIFFDirectory;
>
> So I am guessing when you access td_transferfunction[3], you are
> actually accessing td_refblackwhite, which - surprise surprise - happens
> to be NULL.
While we could certainly do that, I'm not 100% convinced it would be a
good idea, mostly because it would involve a magic number (3), which
we should IMO try to avoid.
Also, if td is spec-compliant, then
td->td_samplesperpixel - td->td_extrasamples
should never be greater than 3 (unless the spec evolves, which is fine
anyways as long as the size of td_transferfunction is re-defined)
because otherwise it would break the assumption
td->td_extrasamples = td->td_samplesperpixel - (standard spp)
which we expect to be guaranteed by the callee.
But, right, something like
/* Should be guaranted by callee */
(td->td_samplesperpixel - td->td_extrasamples) > TD_TRANSFER_FUNCTION_SIZE
and also
typedef struct {
[...]
/* Colorimetry parameters */
uint16* td_transferfunction[TD_TRANSFER_FUNCTION_SIZE];
float* td_refblackwhite;
[...]
} TIFFDirectory;
could clearly be a good idea, at least because it would make the
patch/code clearer on what we are trying to do and what assumption we
make.
Thanks !
Cheers,
Hugo
--
Hugo Lefeuvre (hle) | www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA
Attachment:
signature.asc
Description: PGP signature