[Ffmpeg-devel] [PATCH] BMP encoder

[Michael Niedermayer]

Hi

On Mon, Oct 30, 2006 at 07:01:17PM +0100, Michel Bardiaux wrote:
> Michael Niedermayer wrote:
> >Hi
> >
> >On Mon, Oct 30, 2006 at 05:36:24PM +0100, Michel Bardiaux wrote:
> >>Michael Niedermayer wrote:
> >>>Hi
> >>>
> >>>On Mon, Oct 30, 2006 at 04:05:39PM +0100, Michel Bardiaux wrote:
> >>>>Currently, only straight RGB.
> >>>[...]
> >>>>+    init_put_bits(&s->pb, buf, buf_size);
> >>>>+    // STRUCTURE.field refer to the MSVC documentation
> >>>>+    // BITMAPFILEHEADER.bfType
> >>>>+    put_bits(&s->pb, 8, 'B');
> >>>>+    put_bits(&s->pb, 8, 'M');
> >>>>+    // BITMAPFILEHEADER.bfSize
> >>>>+    write32(&s->pb, n_bytes);
> >>>>+    // BITMAPFILEHEADER.bfReserved1
> >>>>+    write16(&s->pb, 0);
> >>>>+    // BITMAPFILEHEADER.bfReserved2
> >>>>+    write16(&s->pb, 0);
> >>>>+    // BITMAPFILEHEADER.bfOffBits
> >>>>+    write32(&s->pb, 14 /* BITMAPFILEHEADER */ + 40);
> >>>>+    // BITMAPINFOHEADER.biSize
> >>>>+    write32(&s->pb, 40);
> >>>>+    // BITMAPINFOHEADER.biWidth
> >>>>+    write32(&s->pb, avctx->width);
> >>>>+    // BITMAPINFOHEADER.biHeight
> >>>>+    write32(&s->pb, avctx->height);
> >>>>+    // BITMAPINFOHEADER.biPlanes
> >>>>+    write16(&s->pb, 1);
> >>>>+    // BITMAPINFOHEADER.biBitCount
> >>>>+    write16(&s->pb, 24);
> >>>>+    // BITMAPINFOHEADER.biCompression
> >>>>+    write32(&s->pb, BMP_RGB);
> >>>>+    // BITMAPINFOHEADER.biSizeImage
> >>>>+    write32(&s->pb, n_bytes_image);
> >>>>+    // BITMAPINFOHEADER.biXPelsPerMeter
> >>>>+    write32(&s->pb, 0);
> >>>>+    // BITMAPINFOHEADER.biYPelsPerMeter
> >>>>+    write32(&s->pb, 0);
> >>>>+    // BITMAPINFOHEADER.biClrUsed
> >>>>+    write32(&s->pb, 0);
> >>>>+    // BITMAPINFOHEADER.biClrImportant
> >>>see put_bmp_header()
> >>Right, I have recoded the wheel. But I cant use it unchanged; first 
> >>because it is in libavformat, second because it seems intimately linked 
> >>to asf and avi.
> >>
> >>I see 2 ways: recode it in the bmp encoder; or define our own bitmap 
> >>structures to separate setting the values and writing them. For the time 
> >>being I will take the 1st way.
> >
> >hmm ok but please at least dont interleave the write32() and the comments
> >this is confusing, rather put the comments to the right of the write32() 
> >and
> >nicely aligned ...
> >
> >
> >>>
> >>>>+    write32(&s->pb, 0);
> >>>>+    // BMP files are bottom-to-top
> >>>>+    ptr = p->data[0] + (avctx->height - 1) * p->linesize[0];
> >>>>+    linesize = -p->linesize[0];
> >>>>+    for(i = 0; i < avctx->height; i++){
> >>>>+        uint8_t *src = ptr;
> >>>>+        int n = 0;
> >>>>+        for(j=0;j<avctx->width;j++) {
> >>>>+            put_bits(&s->pb, 8, src[0]);
> >>>>+            put_bits(&s->pb, 8, src[1]);
> >>>>+            put_bits(&s->pb, 8, src[2]);
> >>>using the bitstream reader for a purely byte based format is unacceptable
> >>>(reason is that its several times slower and more complex)
> >>Right. Will change to use put_le16 etc. But see above.
> >
> >to put 8bit a simple uint8_t will do ...
> >for 16bit a simple uint16_t with le2me_16() will do as long as its
> >aligned and i think it is ...
> >same for 32bit
> >if its not aligned then there are ST16() ST32() in dsputil.h, they could
> >be moved into a bytesomething.h (in a seperate patch ...)
> 
> Here it is

you are quicker with sending patches then iam with thinking about how to
optimally solve the issues ...

lets see we have 
LD*/ST* from dsputil.h which read and write unaligned native endian
LE_*/BE_* from avcodec.h which read unaligned little and big endian 
bytestream_get_* in bytestream which read unaligned little endian and
increase a pointer

IMHO bytestream.h should only contain functions which increase a pointer
similar to what is currently in there

macros to read and write little,big and native endian should be IMHO in a
seperate file, also LE*/BE*/LD*/ST* should be moved into that and optionally
be cleaned up in the sense that native endian on BE should be equal to BE_*
and similarly on LE
furthermore we could rename them so their names are more consistent
something like:
LDLE_XY load  little endian XY bit
STLE_XY store little endian XY bit
LDBE_XY load  big    endian XY bit
STBE_XY store big    endian XY bit
LD_XY   load  native endian XY bit
ST_XY   store native endian XY bit

or we could leave their names and just add the missing 2
LE_XY   load  little endian XY bit
STLE_XY store little endian XY bit
BE_XY   load  big    endian XY bit
STBE_XY store big    endian XY bit
LDXY    load  native endian XY bit
STXY    store native endian XY bit

comments?

also note the bmp encoder just depends on moving ST* into bytesometing.h
(something!=stream) or writing STLE_* the rest is a seperate issue 

[...]
> @@ -22,6 +22,8 @@
>  #ifndef FFMPEG_BYTESTREAM_H
>  #define FFMPEG_BYTESTREAM_H
>  
> +// These assume alignment

they do not


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

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is