[Ffmpeg-devel] [PATCH] BMP encoder

[Michel Bardiaux]

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

OK, will do that.

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

There was 'previous art': the bmp *decoder* used the bitstreams. There 
was no 'FIXME: change to byte stream for performance'. You're changing 
the rules in the middle of the game. Are you going to reject the patch 
if I do change the decoder to bytestram too? Or reject if I *dont*?

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

Fortunately, they're all aligned (I've checked 3 times).

Do you agree with Baptiste's suggestion to add bytestream_put_byte etc 
to bytestream.h?

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