[FFmpeg-devel] [PATCH] BGR24 Huffyuv and drive-by bug fixes

[Alexander Strange]

On Oct 15, 2009, at 5:53 PM, Michael Niedermayer wrote:

> On Thu, Oct 15, 2009 at 03:51:11PM -0400, Alexander Strange wrote:
>> The Huffyuv decoder currently decodes all RGB files as RGB32 with  
>> the alpha
>> channel left uninitialized - there are rows of 0x80 (the default)  
>> and a few
>> of 0x00 (probably because of plane prediction).
>> This has just recently caused some rendering problems in Perian,  
>> although
>> I'm not sure why it worked before.
>>
>> Attached patches decode 24-bit huffyuv as BGR24 to avoid this, and  
>> fix some
>> other problems I found along the way.
>>
>> 1- fixes a valgrind warning in get_vlc2, caused by the input  
>> padding in
>> bitstream_buffer being uninitialized.
>> 2- fixes temp[0] and temp[1] being allocated when only temp[0] is  
>> used for
>> RGB.
>> 3- adds 'const' to the DSP functions' src pointers (didn't run  
>> patcheck?)
>> and removes a meaningless 'inline'.
>> 4- adds BGR24 decoding for 24-bit files. Decoding speed is about  
>> the same.
>> It requires a bit of copied code; PIX_FMT_RGB32 is endian-dependent  
>> and
>> BGR24 isn't, so merging the two cases turned out very ugly.
>> I used AV_WL32 to do a 24-bit write, since it's faster than AV_WL24  
>> on most
>> platforms.
>> 5- reindents.
>>
>> The last patch is meant to add correct alpha decoding for actual RGBA
>> files, but without a sample I can't test it.
>>
>
>> huffyuv.c |    1 +
>> 1 file changed, 1 insertion(+)
>> b2c3eb9de71f07334e526a66c7020f95bb250fab  0001-Huffyuv-Fix-a- 
>> valgrind-warning-in-get_vlc2.patch
>> From f1e20a1a2db0e19627a60923bbf157e7e0125c0f Mon Sep 17 00:00:00  
>> 2001
>> From: Alexander Strange <astrange at ithinksw.com>
>> Date: Wed, 14 Oct 2009 22:16:28 -0400
>> Subject: [PATCH 1/6] Huffyuv: Fix a valgrind warning in get_vlc2().
>
> ok

Applied.

> [...]
>> huffyuv.c |    4 +---
>> 1 file changed, 1 insertion(+), 3 deletions(-)
>> 0b81e6982f54659034d214368885b7018113e112  0002-Huffyuv-Remove- 
>> unnecessary-allocation-in-alloc_temp.patch
>> From 09cebd6409e9bb3f6b9ad47e99466c908aeac1c5 Mon Sep 17 00:00:00  
>> 2001
>> From: Alexander Strange <astrange at ithinksw.com>
>> Date: Wed, 14 Oct 2009 22:17:15 -0400
>> Subject: [PATCH 2/6] Huffyuv: Remove unnecessary allocation in  
>> alloc_temp().
>>
>> RGB only needs one temp array.
>> ---
>> libavcodec/huffyuv.c |    4 +---
>> 1 files changed, 1 insertions(+), 3 deletions(-)
>>
>> diff --git a/libavcodec/huffyuv.c b/libavcodec/huffyuv.c
>> index 7fd03d8..b8e59ed 100644
>> --- a/libavcodec/huffyuv.c
>> +++ b/libavcodec/huffyuv.c
>> @@ -406,9 +406,7 @@ static av_cold void alloc_temp(HYuvContext *s){
>>             s->temp[i]= av_malloc(s->width + 16);
>>         }
>>     }else{
>> -        for(i=0; i<2; i++){
>> -            s->temp[i]= av_malloc(4*s->width + 16);
>> -        }
>> +            s->temp[0]= av_malloc(4*s->width + 16);
>
> indent is wrong, except that ok

Applied.

>> dsputil.c |    8 ++++----
>> dsputil.h |    8 ++++----
>> 2 files changed, 8 insertions(+), 8 deletions(-)
>> 7234f90d352a3569fd62170e0850a3b1194ae11b  0003-Add-missing-const- 
>> for-src-pointers-in-Huffyuv-dsp-fu.patch
>> From 0cba382264e9717c3461086b1090c0558b440e35 Mon Sep 17 00:00:00  
>> 2001
>> From: Alexander Strange <astrange at ithinksw.com>
>> Date: Thu, 15 Oct 2009 00:26:56 -0400
>> Subject: [PATCH 3/6] Add missing const for src pointers in Huffyuv  
>> dsp functions.
>>
>> Also remove a useless 'inline'.
>
> ok though maybe that should be 2 commits, i see no relation between  
> adding
> const and removing inline

Applied.

>> dsputil.c |   23 +++++++++++++++++++++++
>> dsputil.h |    1 +
>> huffyuv.c |   62 ++++++++++++++++++++++++++++++++++++++ 
>> +-----------------------
>> 3 files changed, 63 insertions(+), 23 deletions(-)
>> b5040e16dc7f1c8edd5b85a99a11f4e32f3ee521  0004-Huffyuv-Decode-RGB24- 
>> as-PIX_FMT_BGR24.patch
>> From 268f8db45761bbf65140c88b6c82d0c2db57225f Mon Sep 17 00:00:00  
>> 2001
>> From: Alexander Strange <astrange at ithinksw.com>
>> Date: Wed, 14 Oct 2009 22:15:28 -0400
>> Subject: [PATCH 4/6] Huffyuv: Decode RGB24 as PIX_FMT_BGR24.
>>
>> Currently it decodes as RGB32 with junk in the alpha channel.
>> ---
>> libavcodec/dsputil.c |   23 ++++++++++++++++++
>> libavcodec/dsputil.h |    1 +
>> libavcodec/huffyuv.c |   62 ++++++++++++++++++++++++++++++ 
>> +------------------
>> 3 files changed, 63 insertions(+), 23 deletions(-)
>
> it might be as fast to handle 3 byte groups in C but its not clear  
> how SIMD
> would behave with that

I don't think it applies here.

The decoder profile looks like:

	73.5%	73.5%	ffmpeg_g	decode_bgr_bitstream
	8.1%	8.1%	ffmpeg_g	add_hfyu_left_prediction_bgr24_c
	1.1%	1.1%	ffmpeg_g	bswap_buf
	1.1%	1.1%	ffmpeg_g	add_bytes_mmx

so it's entirely VLC-lookup bound (on angels_480-huffyuvcompress.avi/ 
x86-64).
The first two functions already can't be easily SIMDed, and the second  
two work just as well in either case.

>> dsputil.c |   11 +++++++++--
>> dsputil.h |    2 +-
>> huffyuv.c |   12 +++++++-----
>> 3 files changed, 17 insertions(+), 8 deletions(-)
>> af61a77328bc033b2a9f9a74535322e51504c1d4  0006-Huffyuv-Decode-the- 
>> alpha-channel-in-RGB32-files.patch
>> From b492be90bf4a0f512daec72b20e53aafbbde7356 Mon Sep 17 00:00:00  
>> 2001
>> From: Alexander Strange <astrange at ithinksw.com>
>> Date: Wed, 14 Oct 2009 22:27:47 -0400
>> Subject: [PATCH 6/6] Huffyuv: Decode the alpha channel in RGB32  
>> files.
>>
>> Untested.
>
> untested is not good ...


I managed to generate a sample with the official encoder  
(camera2_hfyu32.avi and camera2_png.avi in incoming). It works.