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

[Måns Rullgård]

Michael Niedermayer <michaelni at gmx.at> writes:

> 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

I intend to change that.  I foolishly assumed you'd be able to guess
as much.

> You seem to start your argumentation on the need of a change.

There is a lot of room for improvement.  That is reason enough to do it.

> I would prefer if you started with a description of what you plan to
> do or a description of in which way the current system would be in
> your way

The thing getting in my way IS THE LAST INDEX BEING WRONG!!!!

> The coefficients an idct gets are the correct input coefficients

Yes.

> block_last_index is exactly what a optimized idct should check for

Yes, but now it's being given an incorrect value.

-- 
M?ns Rullg?rd
mans at mansr.com