[FFmpeg-devel] [PATCH]levc/hevc_cabac Optimise ff_hevc_hls_residual_coding (especially ARM)

[John Cox]

>John Cox <jc <at> kynesim.co.uk> writes:
>
>> On Tue, 19 Jan 2016 15:59:39 +0000 (UTC), you wrote:
>> 
>> >John Cox <jc <at> kynesim.co.uk> writes:
>> >
>> >> >> +#define UNCHECKED_BITSTREAM_READER 1
>> >> >
>> >> >I don't think that's right, and is a security issue.
>> >> 
>> >> I added that line as (nearly) every other decoder in 
>> >> liavcodec has it -
>> >
>> >Sure?
>> 
>> OK - not all:
>> 
>> h263dec.c
>> h264.c
>> h264_cabac.c
>> h264_cavlc.c
>> huffyuvdec.c
>> ituh263dec.c
>> mpegl2dec.c
>> mpeg12.c
>> mpeg4videodec.c
>> mpeg4video_parser.c
>> 
>> But that probably covers 90% of the video streams 
>> decoded with ffmpeg
>
>The three decoders mpegvideo, h263/asp and h264 are 
>not "(nearly) every other decoder", sorry!

Sorry - I (obviously) misremembered the number of hits I got when I last
did that search.

>> >> in particular h264_cabac.c has it.
>> >
>> >Extensive testing was done before it was added.
>> 
>> Testing that it doesn't seg-fault no matter what the 
>> input or some other sort of testing?
>
>Yes, tests that show that fuzzed input does not crash 
>the decoder are needed.
>
>But afaict, the change is unrelated to the rest of your 
>patch and should be discussed separately (imo).

Yup - perfectly happy to put that can of worms to one side.

>> >Could you confirm how much of the speedup comes 
>> >only from this change?
>> 
>> Not an awful lot - a few % of the total improvement, but 
>> I was looking for everything I can get.  I'll happily 
>> take it out of this patch if it is controversial.
>
>I wouldn't say controversial (I am all for it, sorry if 
>this wasn't clear) but I think it can be discussed after 
>your speedup was committed.

Yup - at this point it is simply a distraction

>> >While we definitely all welcome a noticeable speedup 
>> >of hevc decoding (and while my opinion on your patch 
>> >has limited relevance) I believe that the patch 
>> >absolutely has to be split: First step would be to 
>> >have a split between changes in the general code and 
>> >changes to arm assembly, I believe the first patch 
>> >then may be split further.
>> 
>> Happy to split out the arm asm.
>
>Please do, my suggestion would be to start with the 
>changes to the C code. But it may be wise to wait for a 
>real review first.

I've done enough review processes to know that waiting till the comments
die down before doing anything is the way to go :-)

JC

>Carl Eugen
>
>_______________________________________________
>ffmpeg-devel mailing list
>ffmpeg-devel at ffmpeg.org
>http://ffmpeg.org/mailman/listinfo/ffmpeg-devel