[FFmpeg-devel] [PATCH] encoder for adobe's flash ScreenVideo2 codec

[Vitor Sessak]

Joshua Warner wrote:
> On Tue, Jul 21, 2009 at 11:23 PM, Vitor Sessak<vitor1001 at gmail.com> wrote:
>> Joshua Warner wrote:
>>> Hi all,
>>>
>>> I've developed an encoder for Adobe's Flash ScreenVideo2 format, which is
>>> stored in flv files.  My encoder currently only supports a large subset of
>>> the format.  The only player that supports this codec (so far) is Adobe
>>> Flash Player itself, but ScreenVideo2 makes dramatic improvement in file
>>> size over ScreenVideo (currently in ffmpeg as flashsv) - and should be
>>> very
>>> useful for uploading screencasts, etc.  Most options (block size, etc) now
>>> just fall back on defaults because I couldn't find a general algorithm
>>> that
>>> produced consistantly better results than these.  All the code is in place
>>> to be able to change these parameters dynamically, so future improvements
>>> there should be easy.  The patch is attached.
>>>
>>> The codec is listed as flashsv2 in ffmpeg.
>> I'll give a few comments...

[...]

>>
>>> +static int new_key_frame(FlashSV2Context * s)
>>> +{
>>> +    int i;
>>> +    memcpy(s->keybuffer, s->encbuffer, s->frame_size);
>> Can't this memcpy() be avoided by doing FFSWAP(s->keybuffer, s->encbuffer)
>> at some point?
> FFSWAP only does swapping on fixed-size types (it uses = assignment)

I meant FFSWAP(uint8_t *, s->keybuffer, s->encbuffer). The main idea was 
to just switch pointers to buffers instead of doing a time-consuming memcpy.

>>> +    memcpy(s->key_blocks, s->frame_blocks, s->blocks_size);
>>> +    memcpy(s->key_frame, s->current_frame, s->frame_size);
>> same for those
> dito
>>> +static int write_block(Block * b, uint8_t * buf, int buf_size)
>>> +{
>>> +    int buf_pos = 0;
>>> +    unsigned block_size = b->data_size;
>>> +
>>> +    if (b->flags & HAS_DIFF_BLOCKS)
>>> +        block_size += 2;
>>> +    if (b->flags & ZLIB_PRIME_COMPRESS_CURRENT)
>>> +        block_size += 2;
>>> +    if (block_size > 0)
>>> +        block_size += 1;
>>> +    if (buf_size < block_size + 2)
>>> +        return -1;
>>> +
>>> +    buf[buf_pos++] = block_size >> 8;
>>> +    buf[buf_pos++] = block_size;
>> See AV_WL16().
>>
>>> +
>>> +    if (block_size == 0)
>>> +        return buf_pos;
>>> +
>>> +    buf[buf_pos++] = b->flags;
>>> +
>>> +    if (b->flags & HAS_DIFF_BLOCKS) {
>>> +        buf[buf_pos++] = (uint8_t) (b->start);
>>> +        buf[buf_pos++] = (uint8_t) (b->len);
>>> +    }
>>> +
>>> +    if (b->flags & ZLIB_PRIME_COMPRESS_CURRENT) {
>>> +        //This feature of the format is poorly understood, and as of now,
>>> unused.
>>> +        buf[buf_pos++] = (uint8_t) (b->col);
>>> +        buf[buf_pos++] = (uint8_t) (b->row);
>>> +    }
>>> +
>>> +    memcpy(buf + buf_pos, b->data, b->data_size);
>> Is it really necessary to memcpy() data, or there is a way to write directly
>> to the buffer?
> I have to try several different compression settings.  There would be
> a messy string of dependencies to resolve in order to figure out
> exactly where to write the data to in the buffer (and depending on
> what the ZLIB_PRIME_COMPRESS_CURRENT flag means in the format (that is
> one feature that my encoder doesn't use yet), resolving these
> dependencies at the time of doing the compression might be
> impossible).  No matter what, I have to write to a temporary buffer to
> try these settings anyway.
>>> +
>>> +static int encode_zlib(Block * b, uint8_t * buf, int *buf_size, int comp)
>>> +{
>>> +    int res =
>>> +        compress2(buf, buf_size, b->sl_begin, b->sl_end - b->sl_begin,
>>> +                  comp);
>>> +    return res == Z_OK ? 0 : -1;
>>> +}
>> I think it would be cleaner to use compress2() directly all over the code,
>> for ex. instead of
> I did it this way for two reasons: 1, so that it fits with the error
> passing protocol shared by the rest of the code, and 2, because it
> nicely mirrors encode_zlibprime.  I am an object-oriented programmer
> at heart, and this is (weak) example of encapsulation - only the
> "encoding" code has to know about the details of the zlib interface.
>>> +        res = encode_zlib(b, b->data, &b->data_size, comp);
>>> +        if (res != 0)
>>> +            return res;
>> just
>>
>> if (compress2(b->data, &b->data_size, b->sl_begin,
>>              b->sl_end - b->sl_begin, comp) != Z_OK)
>>    return -1;
>>
>>
>>> +static inline unsigned pixel_bgr(uint8_t * src)
>>> +{
>>> +    return (src[2]) | (src[1] << 8) | (src[2] << 16);
>>> +}
>>
>> Hm, src[0] is unused?
> You are absolutely right - that code is wrong.  I probably didn't
> notice it because the only place it uses that color is in deciding
> whether to use 7-bit paletted color or 15-bit hybrid color.

Note also that this function is useless if you get input as 
PIX_FMT_RGB32 (which is already in native endianness).

-Vitor