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

[Sebastian Vater]

Martin Storsj? a ?crit :
> 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.
>   

Fixed.

> 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.
>   

Fixed.

> 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.
>   

Please review current patch I just attached with this post.

I also now added some more brief documentation to the header file and
fixed some UINTxx_C issues in the new macros.

-- 

Best regards,
                   :-) Basty/CDGS (-

-------------- next part --------------
A non-text attachment was scrubbed...
Name: iff-ham-support.patch
Type: text/x-patch
Size: 17610 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100506/6122fb29/attachment.bin>