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

Fixed by making them typedef enum...

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

Fixed.

>   
>> +#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).
>   

Fixed.

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

Fixed.

> If you insist, you can keep the macros, but at least move the
> GET_EXTRA_DATA() call outside the loop.
>   

Fixed.
>   
>> +        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?)
>   

Fixed.

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

Fixed.

>   
>> +    if (buf_size > 8) {
>>     
>
> If you use if (buf_size < 0) return 0;, you gain 4 spaces along the
> whole rest of the function.
>
>   
>> +            int i;
>> +            int count = FFMIN(GET_EXTRA_SIZE(avctx) / 3, 1 << s->ham);
>>     
>
> You can merge these two lines.
>   

Fixed.

> Rest of patch looks good to me, keep it up!
>   

Please review this one.

It requires the latest byterun1 decoder error handling patch which puts
av_log inside byterun1 decoder function and also the avfilter decoder
crash fix patch.

-- 

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

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