?smail D?nmez wrote:

On Tue, Jul 21, 2009 at 6:01 PM, Mike Melanson<mike at multimedia.cx> 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. How did you figure out the format? I only ask because I don't know anyone who has been able to figure it out based on the official Adobe documentation (that I helped write). :)

Is this a tricky question? ;) To make it harder to reverse engineer :-P

Not exactly. I worked hard during the last revision to clean up the existing SV2 description because we had a number of requests. Combined with the fact that it's very difficult to obtain test media (only generated by another bit of Adobe software), the format was impenetrable. I tried to create a naive encoder (that I wouldn't be able to release) to generate some contrived samples (that I would be able to release) but didn't get too far before I got busy with other things. I'll try to review this patch and see if it exercises all of the codec's features. The codec does feature something I haven't seen elsewhere-- the 15/7-bit hybrid pixel encoding (top bit of a byte indicates whether it's the pixel is RGB555 or a 7-bit palette index). -- -Mike Melanson

On Thu, Jul 23, 2009 at 8:42 AM, Joshua Warner <joshuawarner32 at gmail.com> wrote: [...]

...

...

...

...

I've developed an encoder for Adobe's Flash ScreenVideo2 format [...]

I started on a decoder for this format; attached is my progress so far. It correctly decodes the first frame of samples created by the encoder from this thread, but it does not implement any of the fancy features yet (interframes, diff blocks, custom palettes, zlib priming of either variety). The official documentation seems to be missing a few points or just plain wrong based on comparison with the encoder patch; specifically, ZlibPrimeCompressPrevious (by my reading of this encoder) does not mean that the previous block used zlib priming. It rather means to prime zlib with data from the equivalent block of the previous frame before decompressing the current block's data. This seems like a reinvention of zlib's deflate/inflateSetDictionary functionality... I'm not sure how to prime it on the decompression side (using inflateSetDictionary doesn't work, as far as I can tell, because the stream was not actually compressed with a custom dictionary - the encoder actually runs the previous block through the compressor, discards the result, and then compresses the current block without reinitializing the decoder). It seems like I would need to store the zlib state for each block of the previous frame to make this possible, but surely that is not how it is intended. Anyway, if any intrepid hackers want something to do, have at it - I probably will not do any serious work on it in the near future. Thanks, -- Daniel Verkamp

On Fri, Nov 20, 2009 at 3:00 PM, Michael Niedermayer <michaelni at gmx.at> wrote: [...]

...

...

hmm, what is the problem if no decoder is available? i thought this worked in the past but i might be wrong

[...] and if you simply outcomment the exit call in ffmpeg.c ?

Then it does indeed work (removing the av_exit() call after "could not find codec parameters" message), but presumably it is there for some reason. Is there a way to detect "we don't really need to decode this stream, just demux it"? Thanks, -- Daniel Verkamp

On Fri, Nov 20, 2009 at 3:11 PM, Michael Niedermayer <michaelni at gmx.at> wrote:

On Fri, Nov 20, 2009 at 03:07:11PM -0600, Daniel Verkamp wrote:

...

On Fri, Nov 20, 2009 at 3:00 PM, Michael Niedermayer <michaelni at gmx.at> wrote: [...]

...

...

...

hmm, what is the problem if no decoder is available? i thought this worked in the past but i might be wrong

[...] and if you simply outcomment the exit call in ffmpeg.c ?

Then it does indeed work (removing the av_exit() call after "could not find codec parameters" message), but presumably it is there for some reason.

hmm, maybe the reason is so it is removed? anyway, if you jst remove it the reply ?to your commit will between various personal insults tell us what it was good for ;) i think this would be the most effective way especially as neither of us seems to know what that exit could be good for

I take that back; I tested the ffmpeg.c change on the wrong branch (the one with the decoder patch applied). :) With just the codec id and muxer+demuxer patches, no decoder, it does not work: [flv @ 0x2307b60]dimensions not set Output #0, flv, to '/home/daniel/temp/screen2-copy.flv': Stream #0.0: Video: 0x0000, q=2-31, 200 kb/s, 90k tbn, 1k tbc Stream mapping: Stream #0.0 -> #0.0 Could not write header for output file #0 (incorrect codec parameters ?) Reading the demuxer now, flv doesn't set width/height itself and depends on the codec to set them, hence the codec parameters test fails (rightfully). I guess there is no bug here after all. Thanks, -- Daniel Verkamp

On Tue, Jul 21, 2009 at 9:01 AM, Mike Melanson<mike at multimedia.cx> 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.

How did you figure out the format? I only ask because I don't know anyone who has been able to figure it out based on the official Adobe documentation (that I helped write). :) The official Adobe documentation has MANY holes, not all of which I have been able to bridge. I got sample frames by using Wireshark while playing back an Adobe Bridge recording.

-- ? ?-Mike Melanson _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel at mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

Fran?ois Revol wrote:

...

...

The codec is listed as flashsv2 in ffmpeg. How did you figure out the format? I only ask because I don't know anyone who has been able to figure it out based on the official Adobe documentation (that I helped write). :)

you mean the doc which licence forbids one to implement what it documents ?

Or has the adobe doc licence been sanitized since Rob Savoye's talk at last FOSDEM ?

No such license as of the the v10 updates last year. Try to keep up with current events. And when did specious license restrictions ever stop an FFmpeg dev? :) -- -Mike Melanson

On Tue, Jul 21, 2009 at 01:09:04PM -0600, Joshua Warner wrote:

On Tue, Jul 21, 2009 at 11:46 AM, Diego Biurrun <diego at biurrun.de> wrote:

...

On Tue, Jul 21, 2009 at 08:57:15AM -0600, Joshua Warner wrote:

...

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,

What about writing a decoder first?

I didn't write the decoder first because I never found full example files to test it on (only fragments garnered by monitoring network traffic during an Adobe Breeze recording).? I can test the encoder by playing the resulting files in flash player. Plus, as of now, I have no need for a decoder. I will get to it, eventually...

Maybe Mike can provide you some samples..

...

Your patch is missing a documentation update.

What, exactly needs to be changed here?

Add a line to doc/general.texi that flash sv2 is supported.

...

...

+#define FLASHSV2_DUMB

FLASHSV2_DUMB forces the encoder to use the build-in defaults instead of the experimental "smart" parameter-choosing code, which (as described in my original post) I haven't been able to get it to make decisions that are consistently better than the defaults.? I wanted to leave the "smart" code in place as a starting point for future work on this tangent.? Should I do this differently?

Please add a comment in the code that explains it. Diego

Joshua Warner wrote:
> On Tue, Jul 21, 2009 at 12:13 PM, Daniel Verkamp<daniel at drv.nu> wrote:
>> On Tue, Jul 21, 2009 at 10:57 AM, Joshua Warner<joshuawarner32 at gmail.com> 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.
>>>
>>> Joshua Warner
>> Minor nitpicks:
>>  - "palette" is misspelled as "pallet" throughout
>>  - trailing whitespace on 2 lines
>>  - plenty of warnings from tools/patcheck
> 
> I ran tools/patcheck (which I wasn't aware of before), and most of the
> output was either false positives or would reduce readability of the
> code. 

It does gives a few false positives. All the "x==0 / x!=0 can be 
simplified to !x / x" is a matter of preferred style in FFmpeg (the 
readability depends on how used you are to it). The "missing const" are 
mostly correct.

> I'm not sure what to do about the comments that it complains
> are not doxygen compliant - they are things that don't have any
> applicability outside flashsv2enc.c... should I appease patcheck?

We prefer to add doxy comments for things that would improve the doxygen 
documentation of the file. Typically struct members and comments before 
functions describing what it does.

-Vitor

Joshua Warner wrote:

Him

I fixed the issues you guys have commented on (tell me if I accidentally missed one), and the revised patch is attached.

I'll give a second batch of comments...

+/** + * @file libavcodec/flashsv2enc.c + * Flash Screen Video Version 2 encoder + * @author Joshua Warner + */ + +/* Differences from version 1 stream: + * NOTE: Currently, the only player that supports version 2 streams is Adobe Flash Player itself. + * * Supports sending only a range of scanlines in a block, + * indicating a difference from the corresponding block in the last keyframe. + * * Supports initializing the zlib dictionary with data from the corresponding + * block in the last keyframe, to improve compression. + * * Supports a hybrid 15-bit rgb / 7-bit palette color space. + */ + +/* TODO: + * Don't keep Block structures for both current frame and keyframe. + * Make better heuristics for deciding stream parameters (optimum_* functions). Currently these return constants. + * Figure out how to encode palette information in the stream, choose an optimum palette at each keyframe. + * Figure out how the zlibPrimeCompressCurrent flag works, implement support. + * Find other sample files (that weren't generated here), develop a decoder. + */ + +#include <stdio.h> +#include <stdlib.h>

Are both includes needed?

+#include "avcodec.h" +#include "put_bits.h" +#include "bytestream.h"

Is bytestream.h used?

+static av_cold void cleanup(FlashSV2Context * s) +{ + if (s->encbuffer) + av_free(s->encbuffer);

No need to check if s->encbuffer is null, av_free() already does that.

+static av_cold int flashsv2_encode_init(AVCodecContext * avctx) +{ + FlashSV2Context *s = avctx->priv_data; + + s->avctx = avctx; + + s->comp = avctx->compression_level; + if (s->comp == -1) + s->comp = 9; + if (s->comp < 0 || s->comp > 9) { + av_log(avctx, AV_LOG_ERROR, + "Compression level should be 0-9, not %d\n", s->comp); + return -1; + } + + + if ((avctx->width > 4095) || (avctx->height > 4095)) { + av_log(avctx, AV_LOG_ERROR, + "Input dimensions too large, input must be max 4096x4096 !\n"); + return -1; + } + + if (avcodec_check_dimensions(avctx, avctx->width, avctx->height) < 0) + return -1; + + + s->last_key_frame = 0;

This is unneeded, the context is already alloc'ed with av_mallocz().

+static inline unsigned int chroma_diff(unsigned int c1, unsigned int c2) +{ + unsigned int t1 = (c1 & 0x000000ff) + ((c1 & 0x0000ff00) >> 8) + ((c1 & 0x00ff0000) >> 16); + unsigned int t2 = (c2 & 0x000000ff) + ((c2 & 0x0000ff00) >> 8) + ((c2 & 0x00ff0000) >> 16); + + return abs(t1 - t2) + abs((c1 & 0x000000ff) - (c2 & 0x000000ff)) + + abs(((c1 & 0x0000ff00) >> 8) - ((c2 & 0x0000ff00) >> 8)) + + abs(((c1 & 0x00ff0000) >> 16) - ((c2 & 0x00ff0000) >> 16)); +}

Does doing the square instead of abs() is faster and/or looks better?

+static int optimum_use15_7(FlashSV2Context * s) +{ +#ifndef FLASHSV2_DUMB + double ideal = ((double)(s->avctx->bit_rate * s->avctx->time_base.den * s->avctx->ticks_per_frame)) / + ((double) s->avctx->time_base.num) * s->avctx->frame_number; + if (ideal + use15_7_threshold < s->total_bits) { + return 1; + } else { + return 0; + } +#else + return s->avctx->global_quality == 0; +#endif +}

I think if you were trying to encode optimally (if it's worth the price of been 2x slower), I'd suggest, for each (key?)frame: 1- Encode with 15_7 and see how many bits is consumed (after zlib) and how much distortion (measured, for ex., using chroma_diff()) you get. 2- Encode with bgr and see both the number of bits consumed after zlib and the distortion. Then, you choose the one that has the smallest quantity (distortion + lambda*rate). The reasoning behind that is better explained at doc/rate_distortion.txt. The parameter lambda is found in frame->quality and is passed from the command line by "-qscale" ("-qscale 2.3" => frame->quality == (int) 2.3*FF_LAMBDA_SCALE). It is also a good starting point to implement in future rate control (using VBR with a given average bitrate gives better quality than CBR). Note that what is explained in rate_distortion.txt is already what you are doing with the s->dist parameter (s->dist == 8*lambda), so this "solves" the problem of finding the optimum dist. If the speed loss is not worth the price of trying both methods, I think that s->use15_7 should be chosen set based on frame->quality (by testing on a few samples from what quality value using bgr starts been optimal on average). Unfortunately, the rate distortion method do not solve the problem of finding the optimal block size. How much do quality/bitrate depend on it? -Vitor

On Wed, Jul 22, 2009 at 4:05 PM, Vitor Sessak<vitor1001 at gmail.com> wrote:

Joshua Warner wrote:

...

Him

I fixed the issues you guys have commented on (tell me if I accidentally missed one), and the revised patch is attached.

I'll give a second batch of comments...

...

+/** + * @file libavcodec/flashsv2enc.c + * Flash Screen Video Version 2 encoder + * @author Joshua Warner + */ + +/* Differences from version 1 stream: + * NOTE: Currently, the only player that supports version 2 streams is Adobe Flash Player itself. + * * Supports sending only a range of scanlines in a block, + * ? indicating a difference from the corresponding block in the last keyframe. + * * Supports initializing the zlib dictionary with data from the corresponding + * ? block in the last keyframe, to improve compression. + * * Supports a hybrid 15-bit rgb / 7-bit palette color space. + */ + +/* TODO: + * Don't keep Block structures for both current frame and keyframe. + * Make better heuristics for deciding stream parameters (optimum_* functions). ?Currently these return constants. + * Figure out how to encode palette information in the stream, choose an optimum palette at each keyframe. + * Figure out how the zlibPrimeCompressCurrent flag works, implement support. + * Find other sample files (that weren't generated here), develop a decoder. + */ + +#include <stdio.h> +#include <stdlib.h>

Are both includes needed?

...

+#include "avcodec.h" +#include "put_bits.h" +#include "bytestream.h"

Is bytestream.h used?

I'll make sure to remove the unneeded headers.

...

+static av_cold void cleanup(FlashSV2Context * s) +{ + ? ?if (s->encbuffer) + ? ? ? ?av_free(s->encbuffer);

No need to check if s->encbuffer is null, av_free() already does that.

...

+static av_cold int flashsv2_encode_init(AVCodecContext * avctx) +{ + ? ?FlashSV2Context *s = avctx->priv_data; + + ? ?s->avctx = avctx; + + ? ?s->comp = avctx->compression_level; + ? ?if (s->comp == -1) + ? ? ? ?s->comp = 9; + ? ?if (s->comp < 0 || s->comp > 9) { + ? ? ? ?av_log(avctx, AV_LOG_ERROR, + ? ? ? ? ? ? ? "Compression level should be 0-9, not %d\n", s->comp); + ? ? ? ?return -1; + ? ?} + + + ? ?if ((avctx->width > 4095) || (avctx->height > 4095)) { + ? ? ? ?av_log(avctx, AV_LOG_ERROR, + ? ? ? ? ? ? ? "Input dimensions too large, input must be max 4096x4096 !\n"); + ? ? ? ?return -1; + ? ?} + + ? ?if (avcodec_check_dimensions(avctx, avctx->width, avctx->height) < 0) + ? ? ? ?return -1; + + + ? ?s->last_key_frame = 0;

This is unneeded, the context is already alloc'ed with av_mallocz().

...

+static inline unsigned int chroma_diff(unsigned int c1, unsigned int c2) +{ + ? ?unsigned int t1 = (c1 & 0x000000ff) + ((c1 & 0x0000ff00) >> 8) + ((c1 & 0x00ff0000) >> 16); + ? ?unsigned int t2 = (c2 & 0x000000ff) + ((c2 & 0x0000ff00) >> 8) + ((c2 & 0x00ff0000) >> 16); + + ? ?return abs(t1 - t2) + abs((c1 & 0x000000ff) - (c2 & 0x000000ff)) + + ? ? ? ?abs(((c1 & 0x0000ff00) >> 8) - ((c2 & 0x0000ff00) >> 8)) + + ? ? ? ?abs(((c1 & 0x00ff0000) >> 16) - ((c2 & 0x00ff0000) >> 16)); +}

Does doing the square instead of abs() is faster and/or looks better?

...

+static int optimum_use15_7(FlashSV2Context * s) +{ +#ifndef FLASHSV2_DUMB + ? ?double ideal = ((double)(s->avctx->bit_rate * s->avctx->time_base.den * s->avctx->ticks_per_frame)) / + ? ? ? ?((double) s->avctx->time_base.num) * s->avctx->frame_number; + ? ?if (ideal + use15_7_threshold < s->total_bits) { + ? ? ? ?return 1; + ? ?} else { + ? ? ? ?return 0; + ? ?} +#else + ? ?return s->avctx->global_quality == 0; +#endif +}

I think if you were trying to encode optimally (if it's worth the price of been 2x slower), I'd suggest, for each (key?)frame:

1- Encode with 15_7 and see how many bits is consumed (after zlib) and how much distortion (measured, for ex., using chroma_diff()) you get. 2- Encode with bgr and see both the number of bits consumed after zlib and the distortion.

The only problem with this is that because it is a screen sharing codec, it is important to be able to force it to use lossless mode. Switching modes creates unacceptable artifacts, to not much benefit. At most, I would do this with a number of different s->dist values. This is a good idea for future improvements, but for now, I really need to be able to wrap up this project quickly - I don't have much more time to work on it before I have to begin other projects. Don't think that I'm asking you to accept low-quality code, however.

Then, you choose the one that has the smallest quantity (distortion + lambda*rate). The reasoning behind that is better explained at doc/rate_distortion.txt. The parameter lambda is found in frame->quality and is passed from the command line by "-qscale" ("-qscale 2.3" => frame->quality == (int) 2.3*FF_LAMBDA_SCALE). It is also a good starting point to implement in future rate control (using VBR with a given average bitrate gives better quality than CBR).

Note that what is explained in rate_distortion.txt is already what you are doing with the s->dist parameter (s->dist == 8*lambda), so this "solves" the problem of finding the optimum dist.

If the speed loss is not worth the price of trying both methods, I think that s->use15_7 should be chosen set based on frame->quality (by testing on a few samples from what quality value using bgr starts been optimal on average).

Unfortunately, the rate distortion method do not solve the problem of finding the optimal block size. How much do quality/bitrate depend on it?

Quality doesn't depend at all on the block size, but the bit rate depends a lot on it (I have seen the bit rate change by a factor of 4 between different block sizes). Like I said before, I have tried different formulas for estimating the optimal block size, but none of them have worked consistently better than the 64x64 defaults. Brute force would be the obvious next step, but I think that would be prohibitively expensive, because several frames (probably at least 10% of the inter-key-frame distance) would have to be trial encoded to make a good assessment.

-Vitor _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel at mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

On Wed, Jul 22, 2009 at 11:10:45AM -0600, Joshua Warner wrote:

Him

I fixed the issues you guys have commented on (tell me if I accidentally missed one), and the revised patch is attached.

Applied locally will push to main ffmpeg git it makes no sense to let this rot in the ML archives just because its not perfect [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB When the tyrant has disposed of foreign enemies by conquest or treaty, and there is nothing more to fear from them, then he is always stirring up some war or other, in order that the people may require a leader. -- Plato

On Fri, Apr 29, 2011 at 08:03:01PM -0700, Daniel Verkamp wrote:

On Thu, Apr 28, 2011 at 5:30 PM, Michael Niedermayer <michaelni@gmx.at> wrote:

...

On Wed, Jul 22, 2009 at 11:10:45AM -0600, Joshua Warner wrote:

...

Him

I fixed the issues you guys have commented on (tell me if I accidentally missed one), and the revised patch is attached.

Applied locally will push to main ffmpeg git it makes no sense to let this rot in the ML archives just because its not perfect

Thanks for merging this!

However, a quick test encoding this sample shows a problem: http://drv.nu/temp/flashsv2/screen.flv

iam not sure how to test the resulting file ... what do you use to play it ?

Around 2-4 seconds into the video, some blocks are corrupted.

Reverting 4517ba092e0606e9473c8e148c9895b01f975e18 ("flashsv2enc:fix segfault") fixes the problem for this sample. I could not reproduce the mentioned segfault. I don't understand how that code is supposed to work, but it looks like moving the FFSWAP past the use of s->keybuffer - s->encbuffer will definitely change the results.

without 4517ba092e0606e9473c8e148c9895b01f975e18 s->key_blocks[i].enc += (s->keybuffer - s->encbuffer); will add the pointer difference the wrong way and instead of switching buffers point to random memory. Id guess the most likely way this can work better is by never using the previous data and encoding as keyframe. you can try running the code under valgrind if it doesnt segfault for you. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Democracy is the form of government in which you can choose your dictator

On Fri, May 06, 2011 at 07:33:04PM -0700, Daniel Verkamp wrote:

On Wed, May 4, 2011 at 6:22 AM, Michael Niedermayer <michaelni@gmx.at> wrote:

...

On Fri, Apr 29, 2011 at 08:03:01PM -0700, Daniel Verkamp wrote:

...

On Thu, Apr 28, 2011 at 5:30 PM, Michael Niedermayer <michaelni@gmx.at> wrote:

...

On Wed, Jul 22, 2009 at 11:10:45AM -0600, Joshua Warner wrote:

...

Him

I fixed the issues you guys have commented on (tell me if I accidentally missed one), and the revised patch is attached.

Applied locally will push to main ffmpeg git it makes no sense to let this rot in the ML archives just because its not perfect

Thanks for merging this!

However, a quick test encoding this sample shows a problem:   http://drv.nu/temp/flashsv2/screen.flv

iam not sure how to test the resulting file ... what do you use to play it ?

The standard Adobe Flash player browser plugin plays it fine.

thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Dictatorship naturally arises out of democracy, and the most aggravated form of tyranny and slavery out of the most extreme liberty. -- Plato

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

...

+typedef struct FlashSV2Context { + ? ?AVCodecContext *avctx; + ? ?uint8_t *current_frame; + ? ?uint8_t *key_frame; + ? ?AVFrame frame; + ? ?uint8_t *encbuffer; + ? ?uint8_t *keybuffer; + ? ?uint8_t *databuffer; + + ? ?Block *frame_blocks; + ? ?Block *key_blocks; + ? ?int frame_size; + ? ?int blocks_size; + + ? ?int use15_7, dist, comp; + + ? ?int rows, cols; + + ? ?int last_key_frame; + + ? ?int image_width, image_height; + ? ?int block_width, block_height; + ? ?uint8_t flags; + ? ?uint8_t use_custom_pallet; + ? ?uint8_t pallet_type; ? ? ? ?//0=>default, 1=>custom - changed when pallet regenerated. + ? ?Pallet pallet; + + ? ?double tot_blocks; ? ? ? ? ?//blocks encoded since last keyframe + ? ?double diff_blocks; ? ? ? ? //blocks that were different since last keyframe + ? ?double tot_lines; ? ? ? ? ? //total scanlines in image since last keyframe + ? ?double diff_lines; ? ? ? ? ?//scanlines that were different since last keyframe + ? ?double raw_size; ? ? ? ? ? ?//size of raw frames since last keyframe + ? ?double comp_size; ? ? ? ? ? //size of compressed data since last keyframe + ? ?double uncomp_size; ? ? ? ? //size of uncompressed data since last keyframe + + ? ?double total_bits; ? ? ? ? ?//total bits written to stream so far +} FlashSV2Context;

If it is preferable to use integers here if possible. It is easier to have a bit-exact output across platforms and is faster in systems with no FPU.

...

+static void cleanup(FlashSV2Context * s) +{ + ? ?if (s->encbuffer) + ? ? ? ?av_free(s->encbuffer); + ? ?if (s->keybuffer) + ? ? ? ?av_free(s->keybuffer); + ? ?if (s->databuffer) + ? ? ? ?av_free(s->databuffer); + ? ?if (s->current_frame) + ? ? ? ?av_free(s->current_frame); + ? ?if (s->key_frame) + ? ? ? ?av_free(s->key_frame); + + ? ?if (s->frame_blocks) + ? ? ? ?av_free(s->frame_blocks); + ? ?if (s->key_blocks) + ? ? ? ?av_free(s->key_blocks); +}

Why not simply put this code inside flashsv2_encode_end()? Because it is called to do cleanup if allocation fails in flashsv2_encode_init

...

+static int generate_default_pallet(Pallet * pallet);

You can reorder the functions to avoid this forward reference. This isn't actually needed anyway - I forgot that I moved the call to

On Tue, Jul 21, 2009 at 11:23 PM, Vitor Sessak<vitor1001 at gmail.com> wrote: that function farther down in the file.

...

+static void init_blocks(FlashSV2Context * s, Block * blocks, + ? ? ? ? ? ? ? ? ? ? ? ?uint8_t * encbuf, uint8_t * databuf) +{ + ? ?int row, col; + ? ?Block *b; + ? ?for (col = 0; col < s->cols; col++) { + ? ? ? ?for (row = 0; row < s->rows; row++) { + ? ? ? ? ? ?b = blocks + (col + row * s->cols); + ? ? ? ? ? ?b->width = (col < s->cols - 1) + ? ? ? ? ? ? ? ?|| (s->image_width % s->block_width == + ? ? ? ? ? ? ? ? ? ?0) ? s->block_width : s->image_width % s->block_width;

Hmm, isn't

b->width = (col < s->cols - 1) : ? ? ? ? ? ? ? ?s->block_width ? ? ? ? ? ? ? ? ?s->image_width - col*s->block_width;

simpler?

...

+static av_cold int flashsv2_encode_init(AVCodecContext * avctx) +{ + ? ?FlashSV2Context *s = avctx->priv_data; + + ? ?s->avctx = avctx; + + ? ?s->comp = avctx->compression_level; + ? ?if (s->comp == -1) + ? ? ? ?s->comp = 9; + ? ?if (s->comp < 0 || s->comp > 9) { + ? ? ? ?av_log(avctx, AV_LOG_ERROR, + ? ? ? ? ? ? ? "Compression level should be 0-9, not %d\n", s->comp); + ? ? ? ?return -1; + ? ?} + + + ? ?if ((avctx->width > 4095) || (avctx->height > 4095)) { + ? ? ? ?av_log(avctx, AV_LOG_ERROR, + ? ? ? ? ? ? ? "Input dimensions too large, input must be max 4096x4096 !\n"); + ? ? ? ?return -1; + ? ?} + + ? ?if (avcodec_check_dimensions(avctx, avctx->width, avctx->height) < 0) { + ? ? ? ?return -1; + ? ?} + + + ? ?s->last_key_frame = 0; + + ? ?s->image_width = avctx->width; + ? ?s->image_height = avctx->height; + + ? ?s->block_width = (s->image_width / 12) & ~15; + ? ?s->block_height = (s->image_height / 12) & ~15; + + ? ?s->rows = (s->image_height + s->block_height - 1) / s->block_height; + ? ?s->cols = (s->image_width + s->block_width - 1) / s->block_width; + + ? ?s->frame_size = s->image_width * s->image_height * 3; + ? ?s->blocks_size = s->rows * s->cols * sizeof(Block); + + ? ?s->encbuffer = av_mallocz(s->frame_size); + ? ?s->keybuffer = av_mallocz(s->frame_size); + ? ?s->databuffer = av_mallocz(s->frame_size * 6); + ? ?s->current_frame = av_mallocz(s->frame_size); + ? ?s->key_frame = av_mallocz(s->frame_size); + ? ?s->frame_blocks = av_mallocz(s->blocks_size); + ? ?s->key_blocks = av_mallocz(s->blocks_size);

...

+ ? ?s->pallet.index = av_mallocz(1 << 15);

I think it is simpler to define Pallet as

typedef struct Pallet { ? ?unsigned colors[128]; ? ?uint8_t index[1 << 15]; } Pallet;

?

...

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

...

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

...

+static int generate_default_pallet(Pallet * pallet) +{ + ? ?memcpy(pallet->colors, default_screen_video_v2_palette, + ? ? ? ? ? sizeof(default_screen_video_v2_palette)); +

When using the default palette, it is better to just make pallet->colors point to default_screen_video_v2_palette instead of allocating and memcpy'ing.

I do it this way to support generating a custom palette in the future and still being able to revert to the default palette instead. This is done at most every other key frame (now, its only done once), so the performance impact should be negligible. If I were to just set pallet->colors to default_screen_video_v2_palette, it would needlessly complicate the code to manage the two separate data locations (in reconfigure_at_keyframe).

...

+static int generate_optimum_pallet(Pallet * pallet, uint8_t * image, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? int width, int height, int stride) +{ + ? ?//this isn't implemented yet! ?Default pallet only! + ? ?return -1; +}

I think it would be interesting to handle first the case where the encoder gets the input already as paletted data.

That would completely defeat the purpose of the 15-bit rgb in the hybrid color mode. Given paletted data, it could only effectively use the 7-bit palette half of the hybrid color mode.

...

+#ifndef FLASHSV2_DUMB + ? ?//double save = (1-pow(s->diff_lines/s->diff_blocks/s->block_height, 0.5)) * s->comp_size/s->tot_blocks; + ? ?//double width = block_size_fraction * sqrt(0.5 * save * s->rows * s->cols) * s->image_width; + ? ?//int pwidth; + ? ?//av_log(s->avctx, AV_LOG_DEBUG, "block width: %g\n", width); + ? ?double width; + ? ?width = ((double) s->image_width) / 10.0; + ? ?pwidth = ((int) width); + ? ?pwidth &= ~15; + ? ?if (pwidth > 256) + ? ? ? ?pwidth = 256; + ? ?if (pwidth < 16) + ? ? ? ?pwidth = 16;

Hm, this is the same as

pwidth = FFCLIP((s->image_width/10) & (~15), 16, 256);

I wasn't aware of the FFCLIP macro - I'll change that.

no? Avoid floating point is nice.

At the moment, that code is disabled anyway (it doesn't reliably beat the defaults given in the #else for a wide range of videos). I'll disable (using FLASHSV2_DUMB) the places where the (floating-point) statistics are updated. I used floating point because the "width" variable (now commented out) was my attempt at estimating what an optimum width should be and I designed this estimation by solving for the width when (space savings from not transmitting redundant data) = (space lost from an extra few bytes for each new block). If someone else wants to try and revive this method, great, but its not high on my priorities list.

-Vitor

_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel at mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

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