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

[Michael Niedermayer]

On Mon, Jun 21, 2010 at 03:30:32PM -0700, Jason Garrett-Glaser wrote:
> 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 said i dont understand the issue and that is your explanation?
seriously, iam just asking what you are trying to do that needs the change.
I dont know what you and mans are working on nor do i know your private
code.
Noone has rejected any change, iam asking for further information.
You know, iam the maintainer of that code we talk about and i think
i know it quite well, it would be usefull if you did not try to skip over me
with car analogies.


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

Iam asking you why you need this change
Maybe its needed then its ok
maybe theres a bug in your code and its not needed
maybe theres a cleaner design that avoids it
i dont know without information about what it is that needs that change

for dc only idct the change in question is unneeded
it can be disabled for mpeg2 || mpeg4&&mpeg_quant

and a dc only idct for mpeg2 needs to handle block[63] anyway

what am i missing?

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100622/fce3f120/attachment.pgp>