[FFmpeg-devel] [Patch] QT RLE encoder, bis
Michael Niedermayer
michaelni
Mon Jun 25 00:25:12 CEST 2007
Hi
On Sun, Jun 24, 2007 at 03:56:03PM +0200, Alexis Ballier wrote:
> Hi,
>
> I've been reworking the QT RLE encoder sent months ago [1].
> I should have taken into account the comments, and here is the little story
> :
> I started by implementing the optimal coding algorithm, but it
> happened to be too slow for what I wanted to do with it (screencasts),
> but the code is here and should be correct.
> Then I implemented the heuristic suggested in the last post of [1],
> that seems good to me, and much faster which is perfect for
> screencasts.
>
> I've tried to follow the coding style but as this is my first patch
> for ffmpeg, it's possible that I forgot something, so if it's not ok,
> please tell me what and I'll send another patch.
>
> There are some things that might need comments :
> - I'm using the AVContext rc_max_rate variable to control whether to
> use the optimal coding algorithm or the heuristic, that's the best
> option I found but someone might have a better idea / not like this
> one.
such missuse of rc_max_rate is not ok, there are many better options
also maybe we can simply make the optimal coder fast enough to avoid
the heuristic one ...
> - I've not bumped ffmpeg version in the patch as I don't know how it
> should be changed
> - I've left the copyright headers as in Baptiste's patch in [1], with
> the credits to the original author
>
> My benchs showed that the heuristic is about 10 times faster than the
> optimal but produces files 10 to 20% bigger.
>
> This encoder might support RGB555 pixel format but the current mov
> muxer seems to only allow 24 bits per pixel, so that's disabled. [And
> it will most likely support any pixel format that stores the pixels in
> a variable of size a multiple of 8 bits]
>
> [1]
> http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2007-February/022756.html
[...]
> +/** Maximum RLE codes for bulk copy, repeat and skip */
> +#define MAX_RLE_BULK 127
> +#define MAX_RLE_REPEAT 128
> +#define MAX_RLE_SKIP 254
the doxygen comment does not just apply to the first #define
> +
> +typedef struct QtrleEncContext {
> + AVCodecContext *avctx;
> + AVFrame frame;
> + int pixel_size;
> + uint8_t *previous_frame;
> + unsigned int max_buf_size;
> + /* This array will contain at ith position the value of the best RLE code
> + * if the line started at pixel i
> + * There can be 3 values :
> + * skip (0) : skip as much as possible pixels because they are equal to the
> + * previous frame ones
> + * repeat (<-1) : repeat that pixel -rle_code times, still as much as
> + * possible
> + * copy (>0) : copy the raw next rle_code pixels */
> + uint8_t *rlecode_table;
> + /* This array will contain the length of the best rle encoding of the line
> + * starting at ith pixel */
> + int *length_table;
> +} QtrleEncContext;
comment is not doxygen compatible
[...]
> + s->previous_frame = av_mallocz(avctx->width*avctx->height*s->pixel_size);
this will not work together with a memcpy from a height*linesize frame
> + s->rlecode_table=av_mallocz(s->avctx->width + 1);
> + s->length_table=av_mallocz(s->avctx->width*sizeof(int));
> + if (!s->previous_frame ||
trailing whitespace
[...]
> + for(i=width-1; i>=0; i--) {
> +
> + if((s->frame).key_frame)
> + skipcount=0;
> + else skipcount=scan_equal(this_line, prev_line, FFMIN(width - i,MAX_RLE_SKIP), s->pixel_size, 0);
> +
> + if(skipcount>0)
> + total_skip_cost = (s->length_table[i+skipcount] + 2);
> + else total_skip_cost = width;
> +
> +
> + if(i<width-1) repeatcount=scan_equal(this_line , this_line + s->pixel_size, FFMIN(width-i-1,MAX_RLE_REPEAT-1), s->pixel_size, 0) + 1;
> + else repeatcount=1;
you dont have to run scan_equal() for each pixel, you already know how many
are equal following the next pixel
[...]
> + for(j=2;j<=limit;j++){
> + temp_cost += s->pixel_size;
> + if(s->length_table[i+j] + temp_cost < total_bulk_cost){
> + /* We have found a better bulk copy ... */
> + total_bulk_cost = s->length_table[i+j] + temp_cost;
> + bulkcount = j;
> + }
> + }
its not needed to do this from 2..limit
this can instead be stoped much earlier if its sufficiently worse compared
to any other method
also you dont even need to start the bulk search if there are enough repeats
or skips as the case of repeat/skip + later raw will then always be better
> +
> + if((repeatcount > 1) && (total_repeat_cost < total_bulk_cost)){
> + /* repeat is better than bulk */
> + if((skipcount>0) && (total_skip_cost < total_repeat_cost)){
> + /* skip is the best choice here */
> + s->length_table[i] = total_skip_cost;
> + s->rlecode_table[i] = 0;
> + }
> + else{
> + /* repeat is the best */
> + s->length_table[i] = total_repeat_cost;
> + s->rlecode_table[i] = -repeatcount;
> + }
> + }
> + else{
> + /* bulk is better than repeat */
> + if((skipcount>0) && ( total_skip_cost < total_bulk_cost)){
> + /* skip is the best */
> + s->length_table[i] = total_skip_cost;
> + s->rlecode_table[i] = 0;
> + }
> + else{
> + /* Bulk copy is the best */
> + s->length_table[i] = total_bulk_cost;
> + s->rlecode_table[i] = bulkcount;
> + }
> + }
if(A<B && A<C){
setup stuff for A
}else if(B<C){
setup stuff for B
}else
setup stuff for C
[...]
> + while(i<width){
> + rlecode = (signed char)s->rlecode_table[i];
the cast is unneeded or your code is broken
> + if(rlecode==0){
> + /* Write a skip sequence */
> + bytestream_put_byte(buf, 0);
> + skipcount = scan_equal(this_line+(i*s->pixel_size), prev_line+(i*s->pixel_size), FFMIN(width - i,MAX_RLE_SKIP), s->pixel_size, 0);
> + bytestream_put_byte(buf,skipcount+1);
> + i+=skipcount;
> + }
> + else{
> + if(rlecode>0){
> + /* bulk copy */
> + bytestream_put_byte(buf,rlecode);
> + bytestream_put_buffer(buf,this_line+(i*s->pixel_size), rlecode*s->pixel_size);
> + i+=rlecode;
> + }
> + else{
> + /* repeat the bits */
> + bytestream_put_byte(buf,rlecode);
> + bytestream_put_buffer(buf,this_line+(i*s->pixel_size),s->pixel_size);
> + i-=rlecode;
> + }
> + }
bytestream_put_byte(buf,rlecode);
can be factored out of this loop
[...]
> +/** Dumps frame including header */
> +static int dump_frame(QtrleEncContext *s, AVFrame *p, uint8_t *buf)
encode_frame()
> +{
> + int i;
> + int start_line=0;
> + int end_line=s->avctx->height;
> + uint8_t *orig_buf=buf;
> +
> + if(!((s->frame).key_frame)){
superflous () and there are many more in the patch
[...]
> + if((start_line==0 && end_line == s->avctx->height) || start_line==s->avctx->height)
> + bytestream_put_be16(&buf, 0); // header
> + else {
> + bytestream_put_be16(&buf, 8); // header
> + bytestream_put_be16(&buf, start_line); // starting line
> + bytestream_put_be16(&buf, 0); // unknown
> + bytestream_put_be16(&buf, end_line-start_line); // lines to update
> + bytestream_put_be16(&buf, 0); // unknown
> + }
> + if(s->avctx->rc_max_rate >= 1){
> + for(i=start_line; i < end_line; i++)
> + dump_line_optimal(s, p, i, &buf);
> + }
> + else{
> + for(i=start_line; i < end_line; i++)
> + dump_line_heuristic(s, p, i, &buf);
> + }
indention is inconsistant all over the patch, please use 4 spaces
[...]
> + if (avctx->gop_size == 0 || (s->avctx->frame_number % avctx->gop_size) == 0) {
> + /* I-Frame */
> + p->pict_type = FF_I_TYPE;
> + p->key_frame = 1;
> + chunksize = dump_frame(s, pict, buf);
> + } else {
> + /* P-Frame */
> + p->pict_type = FF_P_TYPE;
> + p->key_frame = 0;
> + chunksize = dump_frame(s, pict, buf);
> + }
chunksize = dump_frame(s, pict, buf);
can be factored out
> +
> + /* save the current frame */
> + memcpy(s->previous_frame, p->data[0], avctx->height*avctx->width*s->pixel_size);
this is wrong if linesize != width*pixel_size
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070625/03335be0/attachment.pgp>
More information about the ffmpeg-devel
mailing list