[FFmpeg-devel] [PATCH] h264: don't touch H264Context->ref_count[] during MB decoding.

[Michael Niedermayer]

On Thu, Oct 04, 2012 at 09:09:22AM +0200, Clément Bœsch wrote:
> On Thu, Oct 04, 2012 at 03:56:12AM +0200, Michael Niedermayer wrote:
> > On Wed, Oct 03, 2012 at 04:25:14PM -0700, Ronald S. Bultje wrote:
> > > From: "Ronald S. Bultje" <rsbultje at gmail.com>
> > > 
> > > The variable is copied to subsequent threads at the same time, so this
> > > may cause wrong ref_count[] values to be copied to subsequent threads.
> > > 
> > > This bug was found using TSAN.
> > > ---
> > >  libavcodec/h264_cabac.c |   41 ++++++++++++++++-------------------------
> > >  libavcodec/h264_cavlc.c |   33 +++++++++++++--------------------
> > >  2 files changed, 29 insertions(+), 45 deletions(-)
> > > 
> > > diff --git a/libavcodec/h264_cabac.c b/libavcodec/h264_cabac.c
> > > index 7e8947b..97e8128 100644
> > > --- a/libavcodec/h264_cabac.c
> > > +++ b/libavcodec/h264_cabac.c
> > > @@ -2003,11 +2003,6 @@ decode_intra_mb:
> > >          return 0;
> > >      }
> > >  
> > > -    if(MB_MBAFF){
> > > -        h->ref_count[0] <<= 1;
> > > -        h->ref_count[1] <<= 1;
> > > -    }
> > > -
> > >      fill_decode_caches(h, mb_type);
> > >  
> > >      if( IS_INTRA( mb_type ) ) {
> > 
> > i suggest to add a priv/local/tls/whatever_ref_count[2]
> > and set/change it as ref_count is before your patch
> > that way the shift doesnt need to be done multiple times in the
> > more inner loops. This should also keep the code smaller and reduce
> > pressure on the code cache
> > 
> 
> Like attached?
> 
> -- 
> Clément B.

>  h264_cabac.c |   36 +++++++++++++++++-------------------
>  h264_cavlc.c |   32 +++++++++++++++-----------------
>  2 files changed, 32 insertions(+), 36 deletions(-)
> 13f9c048ffd86d2f862380d5af7b4402fffeb695  0001-lavc-h264-don-t-touch-H264Context-ref_count-during-M.patch
> From a96e345826b53bf93c6a7c99dc46fd44417679a0 Mon Sep 17 00:00:00 2001
> From: "Ronald S. Bultje" <rsbultje at gmail.com>
> Date: Thu, 4 Oct 2012 09:01:45 +0200
> Subject: [PATCH] lavc/h264: don't touch H264Context->ref_count[] during MB
>  decoding.
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> The variable is copied to subsequent threads at the same time, so this
> may cause wrong ref_count[] values to be copied to subsequent threads.
> 
> This bug was found using TSAN and Helgrind.
> 
> Original patch by Ronald, adapted with a local_ref_count by Clément,
> following the suggestion of Michael Niedermayer.
> 
> Signed-off-by: Clément Bœsch <clement.boesch at smartjog.com>
> ---
>  libavcodec/h264_cabac.c |   36 +++++++++++++++++-------------------
>  libavcodec/h264_cavlc.c |   32 +++++++++++++++-----------------
>  2 files changed, 32 insertions(+), 36 deletions(-)
> 
> diff --git a/libavcodec/h264_cabac.c b/libavcodec/h264_cabac.c
> index a064292..def3624 100644
> --- a/libavcodec/h264_cabac.c
> +++ b/libavcodec/h264_cabac.c
> @@ -1864,6 +1864,9 @@ int ff_h264_decode_mb_cabac(H264Context *h) {
>      int dct8x8_allowed= h->pps.transform_8x8_mode;
>      int decode_chroma = h->sps.chroma_format_idc == 1 || h->sps.chroma_format_idc == 2;
>      const int pixel_shift = h->pixel_shift;
> +    unsigned local_ref_count[2];
> +
> +    memcpy(local_ref_count, h->ref_count, sizeof(local_ref_count));
>  
>      mb_xy = h->mb_xy = s->mb_x + s->mb_y*s->mb_stride;
>  
> @@ -2005,8 +2008,8 @@ decode_intra_mb:
>      }
>  
>      if(MB_MBAFF){
> -        h->ref_count[0] <<= 1;
> -        h->ref_count[1] <<= 1;
> +        local_ref_count[0] <<= 1;
> +        local_ref_count[1] <<= 1;
>      }

maybe
local_ref_count[0] = h->ref_count[0] << MB_MBAFF
is faster but otherwise whatever is fast & clean is fine with me

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Good people do not need laws to tell them to act responsibly, while bad
people will find a way around the laws. -- Plato
-------------- 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/20121004/a66511d5/attachment.asc>