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

[Joshua Warner]

On Wed, Jul 22, 2009 at 9:16 AM, Vitor Sessak<vitor1001 at gmail.com> wrote:
> 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.
I can do it in this case.
>
>>>> + ? ?memcpy(s->key_blocks, s->frame_blocks, s->blocks_size);
>>>> + ? ?memcpy(s->key_frame, s->current_frame, s->frame_size);
>>>
>>> same for those
>>
>> dito
but not for the key_blocks, because I would have to reinitialize them
every key frame then.
>>>>
>>>> +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).
I get input as PIX_FMT_BGR24.
>
> -Vitor
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
>