[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