[FFmpeg-devel] [PATCH] HAM6/HAM8 support for IFF demuxer/decoder
Sebastian Vater
cdgs.basty
Sun May 9 14:30:51 CEST 2010
Ronald S. Bultje a ?crit :
> Hi,
>
> On Sat, May 8, 2010 at 7:07 AM, Sebastian Vater
> <cdgs.basty at googlemail.com> wrote:
>
>> Apart from this, it's pretty the same, hope this patch is ok, so we can
>> finally add it to git/svn to have full HAM support ;)
>>
> [..]
>
>> + if (!st->codec->extradata) {
>> + st->codec->extradata_size = IFF_EXTRA_CONTEXT_SIZE;
>> + st->codec->extradata = av_malloc(IFF_EXTRA_CONTEXT_SIZE + FF_INPUT_BUFFER_PADDING_SIZE);
>> + if (!st->codec->extradata)
>> + return AVERROR(ENOMEM);
>> + }
>> + ex = GET_EXTRA_CONTEXT(st->codec);
>> + memset(ex, 0, IFF_EXTRA_CONTEXT_SIZE);
>>
>
> There's no reason to use GET_EXTRA_CONTEXT() here, since allocation is
> always large enough (and you don't check for NULL anyway).
>
Don't forget that the data is appended to palette due to
backward-compatibility.
So GET_EXTRA_CONTEXT has to calculate the start offset of the actual
data: it's ex = (uint8_t *) extradata + (extradata_size -
FF_EXTRA_CONTEXT_SIZE).
I just think it's also more readable using this macro.
> Then...:
>
>
>> + const uint8_t *ex = GET_EXTRA_CONTEXT(avctx);
>>
>
> Only use of GET_EXTRA_CONTEXT(), so you can now easily integrate that
> macro in this function and remove it.
>
I could do this, but then I have to calculate the offset twice.
>> +/**
>> + * IFF extra context size has to be larger than maximum palette data
>> + * size of previous IFF demuxer and decoder in order to retain
>> + * backward compatibility between an old version of IFF decoder and
>> + * current version of IFF demuxer.
>> + *
>> + * Since 256 palette entries with 3 bytes each color yields in
>> + * 768 bytes, we simply round up to nearest 512 byte block, which
>> + * is exactly 1 KB.
>> + */
>> +#define IFF_EXTRA_CONTEXT_SIZE 1024
>>
>
> Why round to the nearest 512-byte block? It seems to me that
> extradata_size is going to be used for decoder versioning, so you
> wouldn't want to round this, but heave it be exact for all data
> expected in here (so 774 bytes, not 1024).
>
Only for differentiating the old versions of decoder, newer versions
won't increase that anymore, as I described in the header file, I'll
using non 0 tags for it...
Why did I choose 1 KB? It's just a nicer value than 774 and if we ever
need to multiply / divide by it, we can replace these with shifting
instructions.
>> +// Gray to RGB, required for HAM palette table
>> +#define GRAY2RGB(x) (((x) << 16) | ((x) << 8) | (x))
>>
>
> Can the decoder output GRAY8 instead of RGB24 for these frames (or are
> some frames this mode, and others RGB?). Also, this is only used once.
>
It does output GRAY8 for non-HAM...but using HAM doesn't guarantee that
the final image is grayscale (the palette is only used as base then).
Consider this example:
Palette index 7 = 0xCCCCCC...this is a grayscale palette index, right?
Now HAM modifies, after taking palette index 7, the blue component, then
the final output can be 0xCCCCFF or sth. like that, which isn't
grayscale anymore.
The IFF standard itself just says, that you should create a grayscale
colormap if no CMAP chunk is available. It doesn't specifiy that an HAM
image using this has to be grayscale as final image, too...the Amiga
didn't even know of grayscale directly (there aren't any screenmodes for
this).
>
>> +// screenmode bits
>> +#define CAMG_EHB 0x80 // Extra HalfBrite
>> +#define CAMG_HAM 0x800 // Hold And Modify
>> +
>> +// TODO: masking bits
>> +#define MASK_NONE 0
>> +#define MASK_HAS_MASK 1
>> +#define MASK_HAS_TRANSPARENT_COLOR 2
>> +#define MASK_LASSO 3
>>
>
> These (except MASK_NONE) are only used in the decoder and can thus be
> moved out of the header file. I wouldn't mind if you moved MASK_NONE
> also and simply changed the demuxer to init at 0 instead of MASK_NONE,
> with a comment.
>
Fixed.
>
>> + uint8_t compression;
>> + uint8_t ham;
>> + uint8_t ehb;
>> + uint8_t masking;
>> + uint16_t transparency;
>> } IffContext;
>>
>
> Generally, code is faster if you make this native type width, i.e. just int.
>
Fixed. Besides this I changed ham to either 6 or 4 instead of 1 (so I
get rid of all those (bps > 6) ? 6 : 4)
>
>> @@ -290,7 +313,7 @@ static int iff_read_packet(AVFormatContext *s,
>> if(iff->sent_bytes >= iff->body_size)
>> return AVERROR(EIO);
>>
>> - if(s->streams[0]->codec->channels == 2) {
>> + if(st->codec->channels == 2) {
>> uint8_t sample_buffer[PACKET_SIZE];
>>
>> ret = get_buffer(pb, sample_buffer, PACKET_SIZE);
>>
>
> This belongs in its own patch, same for the other ones later in this file.
>
Fixed. Did change this immediately, because with HAM changes, this
brings up an annoying warning that st is unused.
Just forgot to revert this before the HAM patch.
>> @@ -256,13 +285,7 @@ static int iff_read_header(AVFormatContext *s,
>> case AVMEDIA_TYPE_VIDEO:
>> switch (compression) {
>> case BITMAP_RAW:
>> - if (st->codec->codec_tag == ID_ILBM) {
>> st->codec->codec_id = CODEC_ID_IFF_ILBM;
>> - } else {
>> - st->codec->codec_id = CODEC_ID_RAWVIDEO;
>> - st->codec->pix_fmt = PIX_FMT_PAL8;
>> - st->codec->codec_tag = 0;
>> - }
>> break;
>> case BITMAP_BYTERUN1:
>> st->codec->codec_id = CODEC_ID_IFF_BYTERUN1;
>> @@ -299,19 +322,7 @@ static int iff_read_packet(AVFormatContext *s,
>> return AVERROR(ENOMEM);
>> }
>> interleave_stereo(sample_buffer, pkt->data, PACKET_SIZE);
>> - } else if (s->streams[0]->codec->codec_id == CODEC_ID_RAWVIDEO) {
>> - if(av_new_packet(pkt, iff->body_size + AVPALETTE_SIZE) < 0) {
>> - return AVERROR(ENOMEM);
>> - }
>> -
>> - ret = ff_cmap_read_palette(st->codec, (uint32_t*)(pkt->data + iff->body_size));
>> - if (ret < 0)
>> - return ret;
>> - av_freep(&st->codec->extradata);
>> - st->codec->extradata_size = 0;
>> -
>> - ret = get_buffer(pb, pkt->data, iff->body_size);
>> - } else if (s->streams[0]->codec->codec_type == AVMEDIA_TYPE_VIDEO) {
>> + } else if (st->codec->codec_type == AVMEDIA_TYPE_VIDEO) {
>> ret = av_get_packet(pb, pkt, iff->body_size);
>> } else {
>> ret = av_get_packet(pb, pkt, PACKET_SIZE);
>>
>
> So now you're adding "codec handling" for PBM data, right? Is that
> intended? I'd guess that decoding is faster with rawvideo decoder than
> with an actual decoder here.
>
The old code doesn't work anymore with HAM (since you can't use the
CODEC_ID_RAWVIDEO for HAM encoded stuff).
Also it causes a hell of pain in the color map handling since it
requires now to access private data of the codec (also needed for HAM).
Also a demuxer shouldn't directly call decoder functions.
Also note you can't pass the data to rawvideo codec if the PBM uses
masking or transparency color.
> Regardless, this should be its own patch.
>
Not really, because accessing private structures of the decoder was
added with HAM support, so this old-demuxer code simply breaks when
calling read_palette.
>
>> st->codec->height = get_be16(pb);
>> url_fskip(pb, 4); // x, y offset
>> st->codec->bits_per_coded_sample = get_byte(pb);
>> - if (data_size >= 11) {
>> - url_fskip(pb, 1); // masking
>> + if (data_size >= 10)
>> + masking = get_byte(pb);
>> + if (data_size >= 11)
>> compression = get_byte(pb);
>> + if (data_size >= 14) {
>> + url_fskip(pb, 1); // padding
>> + transparency = get_be16(pb);
>> }
>> if (data_size >= 16) {
>> - url_fskip(pb, 3); // paddding, transparent
>> st->sample_aspect_ratio.num = get_byte(pb);
>> st->sample_aspect_ratio.den = get_byte(pb);
>> }
>>
>
> Do files with "random" sizes exist? I'd assume that this block either
> always has >=16 bytes, or 10 bytes (no masing/compress/etc). You could
> make this block simpler then.
>
That's a property of IFF. You shouldn't assume certain sizes, unless
it's clearly described in the standard.
Most amiga IFF readers just read the chunk directly into memory with a
zero-filled data structure. So they won't run into any problem if the
chunk is too small (all non-existing values in the chunk are then
considered to be 0).
And yes, there are files which do this, I know of one IFF-ANIM doing
this (which has a FORM-ILBM chunk embedded as the first frame).
>> - st->codec->extradata_size = data_size;
>>
> [..]
>
>> + st->codec->extradata_size = data_size + IFF_EXTRA_CONTEXT_SIZE;
>>
>
> data_size is the palette size, right? So why is palette_size included
> in IFF_EXTRA_CONTEXT_SIZE also?
>
Because I use this to calculate the palette size (in fact the original
code did this)...by adding data_size to IFF_EXTRA_CONTEXT_SIZE I just
have to subtract IFF_EXTRA_CONTEXT_SIZE to get the actual palette size.
And no, I can't because of compatibility reasons, put a number of
palette entries before the palette data.
>> + const unsigned hbits = bps > 6 ? 6 : 4;
>>
> [..]
>
>> + switch (v >> hbits) {
>> + case 0: // take direct color value from palette
>>
> [..]
>
>> + case 1: // just modify blue color component
>>
> [..]
>
>> + case 2: // just modify red color component
>>
> [..]
>
>> + case 3: // just modify green color component
>>
> [..]
>
>> + }
>>
>
> What if bps<6 and v>>hbits is 4-15? You want ot at least document what
> happens then, or emit an error if it shouldn't.
>
bps == 5 is HAM5 which is declared having a 4bpp color index ,too, but
only with the possibility of modifying the blue component. Same applies
for bps == 7 for HAM7.
If it's <= 4 it is the same as decodeplane8 practically...that's intended.
v >> hbits therefore never can be 4-15...because if planes == 6 then 6-4
= 2 bits for hold mask. If planes == 5 then 5-4 = 1 bit for hold mask.
Hope new attached patch is fine now ;)
--
Best regards,
:-) Basty/CDGS (-:
-------------- next part --------------
A non-text attachment was scrubbed...
Name: iff-ham-support.patch
Type: text/x-patch
Size: 19986 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100509/127a7d3d/attachment.bin>
More information about the ffmpeg-devel
mailing list