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

[Carl Eugen Hoyos]

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!

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

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

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

Carl Eugen