[FFmpeg-devel] [PATCH] mpeg2: fix block_last_index when mismatch control modifies last coeff

[Jason Garrett-Glaser]

On Mon, Jun 21, 2010 at 3:23 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Mon, Jun 21, 2010 at 08:31:28PM +0100, M?ns Rullg?rd wrote:
>> Michael Niedermayer <michaelni at gmx.at> writes:
>>
>> > On Mon, Jun 21, 2010 at 07:12:44PM +0100, M?ns Rullg?rd wrote:
>> >> Michael Niedermayer <michaelni at gmx.at> writes:
>> >>
>> >> > On Mon, Jun 21, 2010 at 08:00:33PM +0200, Michael Niedermayer wrote:
>> >> >> On Mon, Jun 21, 2010 at 05:12:46PM +0100, M?ns Rullg?rd wrote:
>> >> >> > Michael Niedermayer <michaelni at gmx.at> writes:
>> >> >> >
>> >> >> > >> Having an incorrect value in block_last_index just seems
>> >> >> > >> wrong to me.
>> >> >> > >
>> >> >> > > i dont think a completely correct value would be very
>> >> >> > > efficient speedwise as 75% of the cases where the last coeff
>> >> >> > > is set have no effect on the post dct output for dc blocks
>> >> >> > > (that is with an assumtation of dc values all being equally
>> >> >> > > likely)
>> >> >> >
>> >> >> > The last coeff is always manipulated, not only for dc-only blocks. ?Do
>> >> >> > you know for sure exactly in which cases it makes a difference?
>> >> >>
>> >> >> I dont think anything except the idct is using that coeff but i
>> >> >> i cannot formally proof that it is so or is bug free ...
>> >> >> If you have doubts, its surely possible to perform some tests to see
>> >> >> if its value makes a difference outside the idcts
>> >> >
>> >> > Also to everyone to whom things feel semantically wrong
>> >> > one can think of mismatch control to be part of the idct, which isnt
>> >> > far fetched semantically. In that case it makes sense not to update
>> >> > block_last_index. And we then would just be messing with he 64th coeff
>> >> > in the coeff decoder because thats faster there than seperately later.
>> >>
>> >> Sure, but it's hard to optimise the idct properly when the input
>> >> parameters are wrong. ?Choosing various shortcuts based on
>> >> block_last_index is a natural thing to do, but it's not possible when
>> >> this value cannot be trusted.
>> >
>> > That specific idct can be disabled or replaced by something else for mpeg2
>> >
>> > Setting block_last_index "correctly" just means setting it to 63
>> > that will just disable the optimisations
>>
>> Then the "correction" applied to coeff 63 needs to be flagged by some
>> other means. ?Although the coeff decoding is obviously the most
>> efficient place to calculate this adjustment, knowing the actual last
>> coded coeff is useful for the idct. ?Relying on the idct compensating
>> for this munging is IMO totally out of the question.
>
> I dont understand your problem, nor can i relate your rant to the existing
> code
> The code as is, is fast, simple and there are no special cases in any idct
> currently
> You seem to start your argumentation on the need of a change.

We are trying to make things a lot faster.  You are stopping us
because we may need to make something very marginally slower in order
to get the large speed boost.  You're stopping us from adding a larger
engine to a racecar because the bigger exhaust pipe would slightly
increase drag.

I'm trying to merge parts of a local changeset I have that makes the
FLV decoder 30-40% faster overall (many parts may apply to mpegvideo
overall).  Some of these parts are unmergable, but others are quite
mergable.  And you're stopping us from merging them because we might
strip 2 clocks off one function to improve another by 150.

Sometimes one thing has to be made marginally slower to make something
else vastly faster.  I thought you cared about performance -- or do
you only care about keeping things the way they are, however horribly
unoptimized and laughably slow they are?

Dark Shikari