[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 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.

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