[FFmpeg-devel] [PATCH] swscale: unscaled rgb48 <-> bgr48

[Michael Niedermayer]

On Fri, Jun 08, 2012 at 11:40:12AM +0000, Paul B Mahol wrote:
> On 6/8/12, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Fri, Jun 08, 2012 at 10:55:20AM +0000, Paul B Mahol wrote:
> >> Signed-off-by: Paul B Mahol <onemda at gmail.com>
> >> ---
> >>  libswscale/rgb2rgb.c          |   21 ++++++++++++++++++++-
> >>  libswscale/rgb2rgb.h          |    4 ++++
> >>  libswscale/swscale_unscaled.c |   18 +++++++++++++++++-
> >>  3 files changed, 41 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/libswscale/rgb2rgb.c b/libswscale/rgb2rgb.c
> >> index ba7d6de..6b3a01a 100644
> >> --- a/libswscale/rgb2rgb.c
> >> +++ b/libswscale/rgb2rgb.c
> >> @@ -26,6 +26,7 @@
> >>  #include <inttypes.h>
> >>
> >>  #include "libavutil/bswap.h"
> >> +#include "libavutil/intreadwrite.h"
> >>  #include "config.h"
> >>  #include "rgb2rgb.h"
> >>  #include "swscale.h"
> >> @@ -314,7 +315,6 @@ void rgb12tobgr12(const uint8_t *src, uint8_t *dst,
> >> int src_size)
> >>      }
> >>  }
> >>
> >> -
> >>  #define DEFINE_SHUFFLE_BYTES(a, b, c, d)
> >> \
> >>  void shuffle_bytes_ ## a ## b ## c ## d(const uint8_t *src,
> >> \
> >>                                          uint8_t *dst, int src_size)
> >> \
> >> @@ -333,3 +333,22 @@ DEFINE_SHUFFLE_BYTES(0, 3, 2, 1)
> >>  DEFINE_SHUFFLE_BYTES(1, 2, 3, 0)
> >>  DEFINE_SHUFFLE_BYTES(3, 0, 1, 2)
> >>  DEFINE_SHUFFLE_BYTES(3, 2, 1, 0)
> >> +
> >> +#define DEFINE_RGB48TOBGR48(i, o)
> >>    \
> >> +void rgb48tobgr48_ ## i ## o(const uint8_t *src, uint8_t *dst, int
> >> src_size) \
> >> +{
> >>    \
> >> +    uint16_t *d = (uint16_t *)dst;
> >>    \
> >> +    uint16_t *s = (uint16_t *)src;
> >>    \
> >> +    int i, num_pixels = src_size >> 1;
> >>    \
> >> +
> >>    \
> >> +    for (i = 0; i < num_pixels; i += 3) {
> >>    \
> >> +        AV_W ## o ## 16(&d[i    ], AV_R ## i ## 16(&s[i + 2]));
> >>    \
> >> +        AV_W ## o ## 16(&d[i + 1], AV_R ## i ## 16(&s[i + 1]));
> >>    \
> >> +        AV_W ## o ## 16(&d[i + 2], AV_R ## i ## 16(&s[i    ]));
> >>    \
> >> +    }
> >>    \
> >> +}
> >
> > the macro argument "i" and the variable "i" should use different
> > identifiers to avoid unexpected bugs when something more complex is
> > passed in as i
> >
> >
> >> +
> >> +DEFINE_RGB48TOBGR48(L, L)
> >> +DEFINE_RGB48TOBGR48(B, B)
> >> +DEFINE_RGB48TOBGR48(L, B)
> >> +DEFINE_RGB48TOBGR48(B, L)
> >
> > only 2 are needed
> >
> > LL = BB
> > LB = BL
> >
> >
> >
> >> diff --git a/libswscale/rgb2rgb.h b/libswscale/rgb2rgb.h
> >> index c447986..89be977 100644
> >> --- a/libswscale/rgb2rgb.h
> >> +++ b/libswscale/rgb2rgb.h
> >> @@ -52,6 +52,10 @@ extern void (*rgb32tobgr15)(const uint8_t *src, uint8_t
> >> *dst, int src_size);
> >>
> >>  extern void (*shuffle_bytes_2103)(const uint8_t *src, uint8_t *dst, int
> >> src_size);
> >>
> >> +void rgb48tobgr48_LL(const uint8_t *src, uint8_t *dst, int src_size);
> >> +void rgb48tobgr48_BB(const uint8_t *src, uint8_t *dst, int src_size);
> >> +void rgb48tobgr48_LB(const uint8_t *src, uint8_t *dst, int src_size);
> >> +void rgb48tobgr48_BL(const uint8_t *src, uint8_t *dst, int src_size);
> >
> > they should have a sws_ prefix to avoid name clashes with other libs
> 
> All functions in that header do not have any prefix, and I do not
> think prefix is really needed, because
> those functions should be internal only.

they can break any application that itself contains a
rgb48tobgr48_LL() function or any app that links to a 2nd lib
containing such function.
it wont break with every kind of linking (for example versioned shared
libs should be fine) but it will break in many cases
the other non static functions in there should get a prefix as well


> >
> > [...]
> >> @@ -532,7 +539,7 @@ static rgbConvFn findRgbConvFn(SwsContext *c)
> >>      rgbConvFn conv = NULL;
> >>
> >>  #define IS_NOT_NE(bpp, fmt) \
> >> -    (((bpp + 7) >> 3) == 2 && \
> >> +    (bpp <= 16 && \
> >>       (!(av_pix_fmt_descriptors[fmt].flags & PIX_FMT_BE) !=
> >> !HAVE_BIGENDIAN))
> >>
> >>      /* if this is non-native rgb444/555/565, don't handle it here. */
> >
> > this looks unrelated
> 
> It is related, because rgb48be <-> rgb48le would use scaled variant
> which is extremely stupid idea.

maybe iam stupid but neither the code before nor afterwards should
be "true" for bpp=48 but afterwards its true for bpp=8 and lower and
that will cause problems because the PIX_FMT_BE vs. HAVE_BIGENDIAN
comparission will be nonsense


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120608/51eb5c04/attachment.asc>