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

[Måns Rullgård]

Stefano Sabatini <stefano.sabatini-lala at poste.it> writes:

> 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,

And I find it more difficult to understand at a glance, but have it
your way.  Is the write function OK?

-- 
M?ns Rullg?rd
mans at mansr.com