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

[Michael Niedermayer]

Hi

On Thu, May 03, 2007 at 02:45:18PM +0200, Michel Bardiaux wrote:
> 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. 

well count the number of unique palette entries
or do you suggest to add a PAL4, PAL2, PAL1 ?
i think that would be a overkill for these obscure formats


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

rle && 4bit per pixel -> RLE4
rle && 8bit per pixel -> RLE8
so coder type RLE seems enough to me


> 
> >
> >
> >[...]
> >>-    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;

yes


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

the extra () are confusing


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

ive suggested already a few times to change the AVFrame.data[1] palette
format to a non native one, but noone submitted a patch yet ...


[...]

> +        lbuf = buf0 + 14 + ihsize;
> +        for(i = 0; i < 2; i++)
> +            palette[i] = bytestream_get_le32(&lbuf);

you could use buf0 here


> +        for(i = 0; i < avctx->height; i++) {
> +            GetBitContext gb;
> +            lbuf = buf;
> +            init_get_bits(&gb, lbuf, n);
> +            for(j = 0; j < avctx->width; j++)
> +                ptr[j] = get_bits1(&gb);
> +            buf = lbuf + n;

and you could use buf here
which would avoid the lbuf varable

except these patch ok

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No human being will ever know the Truth, for even if they happen to say it
by chance, they would not even known they had done so. -- Xenophanes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070503/d571d17f/attachment.pgp>