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

Ronald S. Bultje rsbultje at gmail.com
Fri Feb 15 21:53:14 CET 2013


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.

Ronald


More information about the ffmpeg-devel mailing list