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

[Michael Niedermayer]

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

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

Avoid a single point of failure, be that a person or equipment.