[FFmpeg-devel] [PATCH] HAM6/HAM8 support for IFF demuxer/decoder

[Martin Storsjö]

On Wed, 5 May 2010, Sebastian Vater wrote:

> Sebastian Vater a ?crit :
> > Because of backward compatibility I have changed it to append the
> > extradata struct after the palette (because in the original version, the
> > palette data was always at 0).
> >   
> 
> I just changed the minimum size of extradata to 256*3 (768 bytes). This
> will allow the new decoder to detect old libavformat/iff.c which doesn't
> set a large palette.
> 
> The palette data from old demuxer was never larger than 256*3 bytes (8
> bit palette, RGB each).

A few comments on this (not a full review):

Why do you include FF_INPUT_BUFFER_PADDING_SIZE in IFF_EXTRA_CONTEXT_SIZE, 
when you later compare extradata_size to IFF_EXTRA_CONTEXT_SIZE? The 
padding should never be included in extradata_size.

And you _still_ cast extradata pointers into a struct pointer, which 
_still_ relies on the struct layout. Even if all the struct members now 
are 8 bit variables, with a lower risk of getting weird padding/alignment, 
you're still relying on undefined behaviour.

So to put it clearly, never use a struct for reading or writing in 
anything that should have a fixed, well-specified format. You may store 
the parsed-out values in structs, but never memcpy/fread/fwrite or cast 
pointers to structs directly, do the reading of each individual value 
separately using constructs that have a well-defined, portable behaviour.

// Martin