[FFmpeg-devel] [PATCH] h264: don't store intra pcm samples in h->mb.

[Michael Niedermayer]

On Fri, Feb 15, 2013 at 12:53:14PM -0800, Ronald S. Bultje wrote:
> Hi,
> 
> On Fri, Feb 15, 2013 at 12:21 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Fri, Feb 15, 2013 at 07:28:38AM -0800, Ronald S. Bultje wrote:
> >> Hi,
> >>
> >> On Feb 14, 2013 7:24 AM, "Ronald S. Bultje" <rsbultje at gmail.com> wrote:
> >> >
> >> > Hi,
> >> >
> >> > On Feb 14, 2013 5:50 AM, "Michael Niedermayer" <michaelni at gmx.at> wrote:
> >> > >
> >> > > On Wed, Feb 13, 2013 at 10:23:47PM -0800, Ronald S. Bultje wrote:
> >> > > > From: "Ronald S. Bultje" <rsbultje at gmail.com>
> >> > > >
> >> > > > Instead, keep them in the bitstream buffer until we read them
> >> verbatim,
> >> > > > this saves a memcpy() and a subsequent clearing of the target buffer.
> >> > > > decode_cabac+decode_mb for a sample file (CAPM3_Sony_D.jsv) goes from
> >> > > > 6121.4 to 6095.5 cycles, i.e. 26 cycles faster.
> >> > > > ---
> >> > > >  libavcodec/h264.h             |  1 +
> >> > > >  libavcodec/h264_cabac.c       |  3 ++-
> >> > > >  libavcodec/h264_cavlc.c       | 11 +++--------
> >> > > >  libavcodec/h264_mb_template.c | 16 ++++++++--------
> >> > > >  4 files changed, 14 insertions(+), 17 deletions(-)
> >> > > >
> >> > > > diff --git a/libavcodec/h264.h b/libavcodec/h264.h
> >> > > > index b0d44cc..ac57658 100644
> >> > > > --- a/libavcodec/h264.h
> >> > > > +++ b/libavcodec/h264.h
> >> > > > @@ -391,6 +391,7 @@ typedef struct H264Context {
> >> > > >      GetBitContext *inter_gb_ptr;
> >> > > >
> >> > > >      DECLARE_ALIGNED(16, int16_t, mb)[16 * 48 * 2]; ///< as a dct
> >> coeffecient is int32_t in high depth, we need to reserve twice the space.
> >> > > > +    const uint8_t *intra_pcm_ptr;
> >> > > >      DECLARE_ALIGNED(16, int16_t, mb_luma_dc)[3][16 * 2];
> >> > > >      int16_t mb_padding[256 * 2];        ///< as mb is addressed by
> >> scantable[i] and scantable is uint8_t we can either check that i is not too
> >> large or ensure that there is some unused stuff after mb
> >> > >
> >> > > the mb writing code can write over the end, thats why mb_padding is
> >> > > there. nothing should be placed between them
> >> >
> >> > But per-mb, aren't the two mutually exclusive and initialized explicitly?
> >>
> >> Ping?
> >
> > iam not sure i understand your question but the way i remember the
> > code, it could write over the mb arries end, and thats why there
> > is something otherwise unused after it.
> > It was designed that way to avoid some checks in the inner loop.
> > mpeg2 is implemented similarly IIRC
> > now if a pointer would be placed between then it could be modifies by
> > an attacker with a crafted stream which would at least crash.
> > That is unless checks where added in the inner loops to avoid that
> > i dont remember such checks being added though but its a long time
> > so i could have forgotten ...
> 
> Right, but to overwrite the pointer, you need to run the coeff reader.
> This isn't done for intra pcm. So for intra pcm, we set the pointer,
> use it, and move on to the next mb. That might be intra pcm or not,
> who knows, but if it is and the previous mb overwrote the poiter,
> that's ok, because we re-set it, use it, and we're moving to the next
> mb.

true, you are correct
but please put the pointer at a less risky place.
It would be like sleeping between 2 railways rails. It IS safe there
unless something is slightly out of place due to whatever

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Asymptotically faster algorithms should always be preferred if you have
asymptotical amounts of data
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130215/e6032772/attachment.asc>