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

[Ronald S. Bultje]

Hi,

On Fri, Feb 15, 2013 at 1:06 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> 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

That was actually funny. :-). OK will move it elsewhere.

Ronald