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

[Michael Niedermayer]

On Tue, Jun 22, 2010 at 12:56:49AM +0100, M?ns Rullg?rd wrote:
> Michael Niedermayer <michaelni at gmx.at> writes:
> 
> > On Mon, Jun 21, 2010 at 11:42:01PM +0100, M?ns Rullg?rd wrote:
> >> Jason Garrett-Glaser <darkshikari at gmail.com> writes:
> >> 
> >> > On Mon, Jun 21, 2010 at 3:23 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> >> >> 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.
> >> 
> >> The corrected block_last_index for mpeg2 will have _no effect at all_
> >> immediately.  When the idct is optimised, it will be faster when a
> >> shortcut is possible, unchanged otherwise.  It is true that without
> >> lots of extra work, mpeg2 will not get the full benefit of such
> >> optimisations.  However, without the fix, mpeg2 decoding will break
> >> with the optimised idct.  
> >
> > why would it break?
> 
> Because it would use invalid shortcuts when coeff 63 has been modified
> without updating block_last_index.
> 
> > I would have expected that we would have a function pointer to a dc idct
> > that could just be made to point to the normal idct for mpeg2
> 
> DC is not the only special case to optimise for.  Which cases are
> possible to shortcut depend on the nature of each optimised idct.  For
> this reason, 


> my current plan is to pass last index to the idct and let
> each implementation do whatever is best based on this.  There will
> thus be no new function pointer.

understood so the issue was bikeshed. Should have been obvious i guess
noone on the ffmpeg team would go to such flaming if it was a real issue

Let me summarize so iam not misunderstanding it
What you suggest is to change block_last_index in decode_block_whatever()
in mpeg2
and then replace the existing idct by one that takes block_last_index too
and does something smart with it assuming block_last_index is strictly
correct
This is easy, optimal for non mpeg2 but not optimal for mpeg2
of course mpeg2 can be optimized too with its own mpeg2 aware idct (that
internally would to large extends branch to existing shared idct code)
and the mpeg2 idct would need the block_last_index in decode_block() reverted
(you didnt say the later but its a logic requirement)
----
what i suggest was to use the new idct(s) only for non mpeg2 for now
and add some idct wraper for mpeg2 that calls another idct with 63 as last
index
This would be easy, optimal for non mpeg2 but not optimal for mpeg2
of course like above, once a volunteer comes along and adds the mpeg2
specific idct the wraper would be replaced by it.

End result 100% identical code
The intermediate in your suggestion might be faster for mpeg2 and is
hacking block_last_index in decode_block() in my oppinion though i guess
you consider it not a hack but the current a hack.

Either way, iam ok with you adding the block_last_index overriding in
decode_block() "hack" until a mpeg2 specific idct is in place or we
come up with a better design that would require a different solution.

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

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- 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/ce518f82/attachment.pgp>