[FFmpeg-devel] [Ffmpeg-devel] [PATCH] non working samples for bmp decoder

[Michel Bardiaux]

Michael Niedermayer wrote:
> Hi
> 
> On Wed, May 02, 2007 at 07:04:53PM +0200, Michel Bardiaux wrote:
>> Compn wrote:
>>> i am just looking for image files that are not supported yet.
>>> here are some bmp files that cause trouble.
>>>
>>> http://samples.mplayerhq.hu/bmp-files/
>>>
>>> 11Bbos20.bmp
>>> [bmp @ 009C8BF0]depth 1 not supported
>> Now readable with the attached patch.
>>
>> I would also like to support various variations (sic) of BMP in the 
>> encoder. What is the best example to follow about how to add options to 
>> a codec?
> 
> the existing -pix_fmt -coder ... should be enough or not?

-pix_fmt supports only PAL8, but bmp's can use depth of 1, 2, 4 or 8. 
And the CODER_TYPE enum does not map well with the coders used in bmp's. 
Eg bmp's have rle8 *and* rle4, which are more complex than simple 
run-length-encoding; and there is the very special BM_BITFIELDS where 
the pixels are actually in the palette (in the file).

> 
> 
> [...]
>> -    n = (avctx->width * (depth / 8) + 3) & ~3;
>> +    n = (((avctx->width * depth) / 8) + 3) & ~3;
> 
> 2 superflous ()

You mean

n = (avctx->width * depth / 8 + 3) & ~3;

is enough? I dont mind, though being *sure* they are equivalent requires 
digging my K&R back up from the year-1991 stratum. But I dont see what 
the gain is.

>>  
>>      switch(depth){
>> +      case 1:
>> +        // 2 palette entries as B,G,R,x in file
>> +        palette = (int*) p->data[1];
>> +        lbuf = buf0 + 14 + ihsize;
>> +        for(i = 0; i < 2; i++){
>> +            palette[i] = bytestream_get_byte(&lbuf); // B
>> +            palette[i] |= bytestream_get_byte(&lbuf)<<8; // G
>> +            palette[i] |= bytestream_get_byte(&lbuf)<<16; // R
>> +            lbuf++;
>> +        }
> 
> the palette code can be written more generically, that is for 1<<depth entries
> 
> bytestream_get_be24()

I dont think that's right. BGR in the file, going through get_be24, 
would give (B<<16)|(G<<8)|R, while the palette is supposed to be 
internally PIX_FMT_RGB32 ie (R << 16) | (G << 8) | B. get_le24 maybe? 
Which would be natural for an MS format after all.

And the 4th byte of the RGBQUAD has to be zero, so get_le32 should be OK.

Pudding-proof: works with attached revised patch.

Which reinforces my opinion that these native-endian pixel formats cause 
more harm than good, since even *you* can be confused!

> 
[snip]

> 
> get_bits1() could be used here (it is likely much faster and more readable)

I'm not sure its faster, since in get_bits1 the fetch is repeated for 
every bit, instead of every 8 bits. But it is certainly more readable, 
so done.

[snip]

> 
> the indention of case missmatches

Fixed

-- 
Michel Bardiaux
R&D Director
T +32 [0] 2 790 29 41
F +32 [0] 2 790 29 02
E mailto:mbardiaux at mediaxim.be

Mediaxim NV/SA
Vorstlaan 191 Boulevard du Souverain
Brussel 1160 Bruxelles
http://www.mediaxim.com/
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: lavc_bmp_depth1_2.pat
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070503/f7edf273/attachment.asc>