[FFmpeg-devel] [PATCH] pixdesc: use 8-bit accesses when possible in av_read/write_image_line()

[Michael Niedermayer]

On Sun, Sep 12, 2010 at 10:43:16PM +0200, Stefano Sabatini wrote:
> On date Sunday 2010-09-12 21:20:52 +0100, M?ns Rullg?rd encoded:
> > Stefano Sabatini <stefano.sabatini-lala at poste.it> writes:
> > 
> > > On date Sunday 2010-09-12 16:59:40 +0100, Mans Rullgard encoded:
> > >> This fixes out of bounds accesses for big endian formats and should be
> > >> a little faster.
> > >> ---
> > >>  libavutil/pixdesc.c |   20 ++++++++++++++++++++
> > >>  1 files changed, 20 insertions(+), 0 deletions(-)
> > >> 
> > >> diff --git a/libavutil/pixdesc.c b/libavutil/pixdesc.c
> > >> index e2bc649..41bf717 100644
> > >> --- a/libavutil/pixdesc.c
> > >> +++ b/libavutil/pixdesc.c
> > >> @@ -54,6 +54,17 @@ void av_read_image_line(uint16_t *dst, const uint8_t *data[4], const int linesiz
> > >>      } else {
> > >>          const uint8_t *p = data[plane]+ y*linesize[plane] + x*step + comp.offset_plus1-1;
> > >>  
> > >> +        if (shift + depth <= 8) {
> > >> +            p += !!(flags & PIX_FMT_BE);
> > >> +            while(w--) {
> > >> +                int val = *p;
> > >> +                val = (val>>shift) & mask;
> > >> +                if(read_pal_component)
> > >> +                    val= data[1][4*val + c];
> > >> +                p+= step;
> > >> +                *dst++= val;
> > >> +            }
> > >> +        } else {
> > >>          while(w--){
> > >>              int val;
> > >>              if(flags & PIX_FMT_BE) val= AV_RB16(p);
> > >> @@ -64,6 +75,7 @@ void av_read_image_line(uint16_t *dst, const uint8_t *data[4], const int linesiz
> > >>              p+= step;
> > >>              *dst++= val;
> > >>          }
> > >> +        }
> > >>      }
> > >>  }
> > >
> > > I'd like to avoid the code duplication.
> > > What about something like this:
> > >
> > > diff --git a/libavutil/pixdesc.c b/libavutil/pixdesc.c
> > > index e2bc649..3496787 100644
> > > --- a/libavutil/pixdesc.c
> > > +++ b/libavutil/pixdesc.c
> > > @@ -52,12 +52,16 @@ void av_read_image_line(uint16_t *dst, const uint8_t *data[4], const int linesiz
> > >              *dst++= val;
> > >          }
> > >      } else {
> > > +        int is_8bit = 0;
> > >          const uint8_t *p = data[plane]+ y*linesize[plane] + x*step + comp.offset_plus1-1;
> > >
> > > +        if (shift + depth <= 8) {
> > > +            p += !!(flags & PIX_FMT_BE);
> > > +            is_8bit = 1;
> > > +        }
> > >          while(w--){
> > >              int val;
> > > -            if(flags & PIX_FMT_BE) val= AV_RB16(p);
> > > -            else                   val= AV_RL16(p);
> > > +            val = is_8bit ? *p : flags & PIX_FMT_BE ? AV_RB16(p) : AV_RL16(p);

this would benefit from if() or \n


> > >              val = (val>>shift) & mask;
> > >              if(read_pal_component)
> > >                  val= data[1][4*val + c];
> > > ?
> > 
> > I don't like putting conditional things in tight loops.  GCC has a
> > nasty habit of not optimising it properly.  In this case it also makes
> > the whole thing look more convoluted than it really is.  The code can
> > be reduced a little though:
> > 
> >             while (w--) {
> >                 int val = (*p>>shift) & mask;
> >                 if (read_pal_component)
> >                     val = data[1][4*val + c];
> >                 p += step;
> >                 *dst++ = val;
> >             }
> > 
> > Besides, I don't think a couple of duplicated lines are anything to
> > fret over.
> 
> I still prefer the version with less lines of code as I found it more
> readable, also note that this function is mainly supposed to be a
> testing device, so efficiency considerations are not so important.

i agree, i also would like to keep this code simple and short

also if we do go the way of this patch, is_8bit could be in the flags


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

Awnsering whenever a program halts or runs forever is
On a turing machine, in general impossible (turings halting problem).
On any real computer, always possible as a real computer has a finite number
of states N, and will either halt in less than N cycles or never halt.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100913/77b65ad1/attachment.pgp>