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

[Martin Storsjö]

Hi,

On Tue, 4 May 2010, Sebastian Vater wrote:

> Sebastian Vater a ?crit :
> > Sebastian Vater a ?crit :
> >   
> >> Peter Ross a ?crit :
> >>> Yep, just allocate a fixed length of bytes (for the flags) at the start
> >>> of the extradata buffer and then append palette.
> >>>       
> >> Fixed! Please review attached patch.

First of all, please trim your quotes when replying, just keep the parts
that you are replying to.

> +    ex = (IffExtraContext *) st->codec->extradata;
> +    memset(ex, 0, sizeof(IffExtraContext));
> +    ex->compression = compression;
> +
>      switch(st->codec->codec_type) {
>      case AVMEDIA_TYPE_AUDIO:
>          av_set_pts_info(st, 32, 1, st->codec->sample_rate);
> @@ -220,6 +240,7 @@ static int iff_read_header(AVFormatContext *s,
>          break;
> 
>      case AVMEDIA_TYPE_VIDEO:
> +        ex->screenmode = screenmode;
>          switch (compression) {
>          case BITMAP_RAW:
>              if (st->codec->codec_tag == ID_ILBM) {

Here, you're making the extradata format totally dependent on the layout 
of the IffExtraContext struct. If you upgrade libavcodec to another 
version, where this struct has changed, things will break. Implementations 
on different architectures with different endianess, word lengths or 
struct packings will have incompatible extradata formats. You should be 
able to do the demuxing on one machine and the decoding (given codec 
parameters, AVPackets and extradata) on a completely different machine.

Therefore, the extradata format for a codec should be fixed and 
well-defined, since it is part of the external interface for anybody using 
libavcodec (which is a whole lot more than just libavformat and ffmpeg.c).

So instead of using a struct for that, manually pack and unpack the values 
that you need into a few bytes that you've allocated and kept track of. 
Try to make sure that you won't need to change the format, to keep 
compatibility.

// Martin