[FFmpeg-devel] [PATCH] HAM6/HAM8 support for IFF demuxer/decoder
Thu May 6 17:35:04 CEST 2010
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.
More information about the ffmpeg-devel