[FFmpeg-devel] [PATCH] IFF: Add the HAM stuff

[Sebastian Vater]

Ronald S. Bultje a ?crit :
> Hi,
>
> On Sun, May 16, 2010 at 11:19 PM, Sebastian Vater
> <cdgs.basty at googlemail.com> wrote:
>   
>> +++ b/libavcodec/iff.c
>>     
> [..]
>   
>> +// TODO: masking bits
>> +#define MASK_NONE                  0
>> +#define MASK_HAS_MASK              1
>> +#define MASK_HAS_TRANSPARENT_COLOR 2
>> +#define MASK_LASSO                 3
>>     
>
> There aren't used, only in error handling. As said before, I'd
> recommend adding this when you're adding the functionality. Right now,
> an if (masking) { av_log_missing_feature(); return error; } is all you
> need.
>   

Ok NONE is obselete now as well as is LASSO...I have only to check
HAS_MASK || HAS_TRANSPARENT_COLOR because they actually change output of
an image (i.e. not handling them causes possible wrong display of image
data).
MASK_LASSO is only becoming interesting if you want to merge this image
with another (i.e. copy it into another), does FFmpeg even support such
stuff, anyway?
With merge I mean stuff like, copy src to (32,32-64,64) of dst with the
ability of keeping certain bitplanes or change others...

Anyway, why I added them all actually at once is, I want just (or others
maybe as well?) be remembered that there are more mask flags...by a
simple search in FFmpeg iff.* source code they just recognize that
nothing except MASK_LASSO is supported right now...

Well, the more I'm thinking about this...IFF-ANIM could just do that!

It could just tell the next frame, copy from last src from 64<=x<=192 
using only planes 2, 3 and 5 and store 0xFFFF in all the others (i.e. 1,
4, and any 6 above)...

But as already said, I'm not really sure about how to handle lasso right
now...

> Separately, this should be an enum, and might require some
> documentation, e.g. what is a lasso? (Note how this totally gets into
> the actual technical details of how you'd implement this patch, which
> is why I'd add this only when you add that, not before.)
>   

MASK_LASSO isn't really a thing I have concerned now,
I just added it for sake of completenessly...and as a hint of reminder
that it may be missing (depending also on FFmpeg's image features, can
it merge two images, i.e. display them together over each other...it's
only these cases where MASK_LASSO really is a difference).

>   
>> +    unsigned  bpp;          ///< bits per plane to decode
>>     
>
> I'd add some documentation indicating that this is NOT always the same
> as avctx->bits_per_coded_sample, because I was confused about that,
> and then indicating how the two differ.
>   

Oh that's a good point to mention...bpcs is always target plane size,
while bpp is always the one of the source. Will fix tomorrow!

>   
>> +#define GET_EXTRA_DATA(x) ((uint8_t *) (x)->extradata + AV_RB16((x)->extradata))
>>     
> [..]
>   
>> +#define GET_EXTRA_SIZE(x) ((x)->extradata_size - AV_RB16((x)->extradata))
>>     
> [..]
>   
>> +#define GET_PACKET_DATA(x) ((uint8_t *) (x)->data + AV_RB16((x)->data))
>>     
> [..]
>   
>> +#define GET_PACKET_SIZE(x) ((x)->size - AV_RB16((x)->data))
>>     
>
> I'm OK with this, but would like to mention that this borderlines
> overengineering ..
>
> If you intend to keep them, please rename the EXTRA_DATA/SIZE to
> PALETTE_DATA/SIZE (it's not "extra" data, it's a palette).
>   

Hmm, wouldn't be this change alone just be inconsequent? I mean now I
have EXTRA and PACKET (avctx->extradata and avpkt->data), but replacing
one with PALETTE but keep the other with PACKET would be inconsistent,
the PACKET stuff should then be called sth. like: PIXEL

>   
>>      if (count) {
>>          for (i=0; i < count; i++) {
>> -            pal[i] = 0xFF000000 | AV_RB24( avctx->extradata + i*3 );
>> +            pal[i] = 0xFF000000 | AV_RB24(GET_EXTRA_DATA(avctx) + i*3);
>>          }
>>      } else { // Create gray-scale color palette for bps < 8
>>     
>
> .. and this is why. You're calculating the actual palette location in
> every run in the loop, which isn't optimal. You could add a uint8_t
> *ptr = GET_EXTRA_DATA(avctx) at the beginning of the loop, so you
> don't recalculate it inside the loop.
>   

Oh here I had to laugh actually, wasn't me told by somebody (*wink* @
Mans) I shouldn't care toooo much about micro-ops?

Well, I exceptionally did that here, because that function is called
once per frame (that is if we have a 50Hz refresh rate video which does
use full screen refresh, it would just be called 50 times a second).

The overhead is really small (on BE it's just two MOV's), on LE it's two
MOV and one ROL. So I really didn't care about this on that special case.

But on the other way, I also agree, you're right, if we start to
optimize and do such stuff, it should be done anywhere...so I will fix
this tomorrow, too!

> Now, I want to reiterate, again, that this sort of poor design is
> common when things are overengineered. This is why I'm personally not
> in favor of overengineering, it leads to poorer performance and you
> generally don't even notice because you're buried in macros, wrappers
> and conveniences to work around their general and implicit
> awkwardness.
>
> if (extradata_size < 2) return error;
> uint8_t *palette_data = avctx->extradata      + AV_RB16(avctx->extradata);
> int palette_size      = avctx->extradata_size - AV_RB16(avctx->extradata);
>
> That isn't so bad, is it? It's also more descriptive.
>
> [..]
>
> If you insist, you can keep the macros, but at least move the
> GET_EXTRA_DATA() call outside the loop.
>   

Will fix tomorrow (see above and below).

>   
>> +        if (buf_size <= 1 || (int) GET_PACKET_SIZE(avpkt) <= 1) {
>>     
>
> The cast is unneeded, extradata_size is signed by itself. (Note again
> how the wrapper macro hides that fact, adding to the total awkwardness
> of the whole?)
>   

extradata_size? This is size of AVPacket...I expected them unsigned,
though...

>   
>> +            av_log(avctx, AV_LOG_ERROR,
>> +                   "Invalid packet size received: %u -> video data offset: %d\n",
>> +                   (unsigned) buf_size, (int) GET_PACKET_SIZE(avpkt));
>>     
>
> Both casts are unneeded, buf_size is already unsigned and
> extradata_size is already signed, and their signedness is already
> indicated by the printf() string.
>   

Will fix that tomorrow...

>   
>> +        buf = avctx->extradata;
>> +        buf_size = bytestream_get_be16(&buf);
>> +        if (buf_size <= 1 || (int) GET_EXTRA_SIZE(avctx) < 0) {
>> +            av_log(avctx, AV_LOG_ERROR,
>> +                   "Invalid extradata size received: %u -> palette data offset: %d\n",
>> +                   (unsigned) buf_size, (int) GET_EXTRA_SIZE(avctx));
>> +            return AVERROR_INVALIDDATA;
>>     
>
> Duplicate. It's probably easier if you call this function with
> avpkt->data and avpkt->size or extradata and size, the caller knows
> which of the two is intended and then this duplicate code can be
> merged into one.
>   

The thing is that both can be invalid though, either who calls them (all
that depends on the demuxer or a cracker trying to cause buffer overflow
with such stuff).

>   
>> +    if (buf_size > 8) {
>>     
>
> If you use if (buf_size < 0) return 0;, you gain 4 spaces along the
> whole rest of the function.
>   

Only until next update, then there will be like in the IFF demuxer:
if (buf_size > 8) {
...
}

if (buf_size > 14) {
...
}

That was really intended and so I did it straight on the right way,
before having to reindent that later.

Or to be simple, it will make future patches more complicated.

>   
>> +            int i;
>> +            int count = FFMIN(GET_EXTRA_SIZE(avctx) / 3, 1 << s->ham);
>>     
>
> You can merge these two lines.
>   

Oh ok...I just don't really like to merge uninitialized variables with
initialized in one line...

>   
>> +                s->ham_palbuf[(i+count)*2]     = 0x00FFFF; // just modify blue color component
>> +                s->ham_palbuf[(i+count*2)*2]   = 0xFFFF00; // just modify red color component
>> +                s->ham_palbuf[(i+count*3)*2]   = 0xFF00FF; // just modify green color component
>> +                s->ham_palbuf[(i+count)*2+1]   = tmp << 16;
>> +                s->ham_palbuf[(i+count*2)*2+1] = tmp;
>> +                s->ham_palbuf[(i+count*3)*2+1] = tmp << 8;
>>     
>
> Vertical align.
>   

You mean a blank space between tmp << 16 and the line before?

>> +    const uint8_t *buf = avpkt->size >= 2 ? GET_PACKET_DATA(avpkt) : NULL;
>> +    const int buf_size = avpkt->size >= 2 ? GET_PACKET_SIZE(avpkt) : 0;
>>     
>
> Both a >= b ? c : d are unneeded, since you're checking each touch of
> buf against buf_end, so if it's out of bounds (regardless of whether
> buf == buf_end of buf > buf_end), you'll never read *buf, and buf_size
> is unused otherwise.
>   

Both macros access AV_RB16(avpkt->data)...there fore they have to ensure
that avpkt->size is at least >= 2...or doesn't that matter because of
AVPacket data buffer padding?
If this pads these are really unnecessary since extract_header checks
for correctness...

>> +    const uint8_t *buf = avpkt->size >= 2 ? GET_PACKET_DATA(avpkt) : NULL;
>> +    const int buf_size = avpkt->size >= 2 ? GET_PACKET_SIZE(avpkt) : 0;
>>     
>
> Same.
>
> Rest of patch looks good to me, keep it up!
>   

Thank you, really want to do it right now, but I guess that when I try
that in my tired state now, the result will be just plain crap, so let's
do that tomorrow, ok?

-- 

Best regards,
                   :-) Basty/CDGS (-: