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

[Michael Niedermayer]

On Mon, Jun 21, 2010 at 08:34:53AM -0700, Jason Garrett-Glaser wrote:
> On Mon, Jun 21, 2010 at 8:32 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Mon, Jun 21, 2010 at 03:35:13PM +0100, M?ns Rullg?rd wrote:
> >> Michael Niedermayer <michaelni at gmx.at> writes:
> >>
> >> > On Mon, Jun 21, 2010 at 02:19:22PM +0100, M?ns Rullg?rd wrote:
> >> >> Michael Niedermayer <michaelni at gmx.at> writes:
> >> >>
> >> >> > On Mon, Jun 21, 2010 at 11:10:55AM +0100, M?ns Rullg?rd wrote:
> >> >> >> Michael Niedermayer <michaelni at gmx.at> writes:
> >> >> >>
> >> >> >> > On Sun, Jun 20, 2010 at 07:35:16PM -0700, Jason Garrett-Glaser wrote:
> >> >> >> >> On Sun, Jun 20, 2010 at 7:00 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> >> >> >> >> > On Mon, Jun 21, 2010 at 02:47:16AM +0100, M?ns Rullg?rd wrote:
> >> >> >> >> >> Michael Niedermayer <michaelni at gmx.at> writes:
> >> >> >> >> >>
> >> >> >> >> >> > On Mon, Jun 21, 2010 at 12:40:14AM +0100, M?ns Rullg?rd wrote:
> >> >> >> >> >> >> Michael Niedermayer <michaelni at gmx.at> writes:
> >> >> >> >> >> >>
> >> >> >> >> >> >> > On Sun, Jun 20, 2010 at 11:41:32PM +0100, Mans Rullgard wrote:
> >> >> >> >> >> >> >> - ? ?s->block_last_index[n] = i;
> >> >> >> >> >> >> >> + ? ?s->block_last_index[n] = block[63]? 63: i;
> >> >> >> >> >> >> >> ? ? ?return 0;
> >> >> >> >> >> >> >
> >> >> >> >> >> >> > is this fixing a bug?
> >> >> >> >> >> >>
> >> >> >> >> >> >> The value of block_last_index is wrong otherwise. ?We're trying to add
> >> >> >> >> >> >> a dc-only idct (and perhaps other long zero tails). ?That requires a
> >> >> >> >> >> >> correct block_last_index value.
> >> >> >> >> >> >>
> >> >> >> >> >> >> > because, if it makes no difference, then it would cause a
> >> >> >> >> >> >> > speedloss and possibly not small if this is a conditional
> >> >> >> >> >> >> > branch that fails to be predicted
> >> >> >> >> >> >>
> >> >> >> >> >> >> Do you have a better suggestion?
> >> >> >> >> >> >
> >> >> >> >> >> > ignore it?
> >> >> >> >> >>
> >> >> >> >> >> Then the optimisation is impossible.
> >> >> >> >> >
> >> >> >> >> > you are unable to ignore block[63] in the dc idct ?
> >> >> >> >>
> >> >> >> >> And have non-bit-exact decoding?
> >> >> >> >
> >> >> >> > for which dc value is it non bitexact?
> >> >> >>
> >> >> >> RTFS, all even values.
> >> >> >
> >> >> > before the (reference) idct but not after it AFIAK
> >> >>
> >> >> Please elaborate.
> >> >
> >> > Iam an idiot
> >> > I thought the small contribution of the 64th coeff being 1 would
> >> > make no difference unless something ended pretty close to 0.5 and
> >> > i further thought that the 1 case didnt occur for such dc values.
> >> > I even tested it quickly but messed up again, -2 vs. -3 ...
> >> > i retested it now and as long as
> >> > dc % 8 != 4 the 64th coeff being 1 for even makes no difference
> >> > for dc only blocks
> >> >
> >> > either way, a (block[63]&7) == 4 test before the dc idct seems
> >> > better to me than updating last_index for all even dc values as
> >> > that way the dc idct can be used for more cases
> >>
> >> 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)
> >
> >
> >>
> >> Also, this mismatch control thing is only used in mpeg2. ?Extra checks
> >> elsewhere would needlessly penalise other codecs.
> >
> > mpeg4 asp can depending on a flag (mpeg_quant) also use mpeg2 dequantization
> > (with its mismatch control afiak) and its used in the wild, dunno how common
> > though.
> > The mpeg_quant codepath in mpeg4 is not strongly optimized though so it
> > uses the mpeg2 dequantization instead of having it merged into the coeff
> > decoder.
> > If someone cared this could be changed of course, at the expense of a bit more
> > code it would be closer to mpeg2
> 
> We could have a different idct_dc function pointer that's loaded when
> mpeg quant is on, which checks the last coefficient as well.  It could
> also optimize for that case.

this seems the best idea so far ...

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Dictatorship naturally arises out of democracy, and the most aggravated
form of tyranny and slavery out of the most extreme liberty. -- Plato
-------------- 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/20100621/cdcbcbd6/attachment.pgp>