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

Stefano Sabatini stefano.sabatini-lala
Sun Sep 12 22:43:16 CEST 2010


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

Regards.
-- 
FFmpeg = Fundamental Faithful Mastodontic Powerful Experimenting Gymnast



More information about the ffmpeg-devel mailing list