[FFmpeg-devel] make swscale's palette functions public
Michael Niedermayer
michaelni
Sun May 16 20:58:36 CEST 2010
On Sun, May 16, 2010 at 06:28:45PM +0200, Reinhard Tartler wrote:
> On Sun, May 16, 2010 at 13:39:28 (CEST), Michael Niedermayer wrote:
>
> > On Sat, May 15, 2010 at 06:39:59PM +0200, Reinhard Tartler wrote:
> >> On Sat, May 15, 2010 at 17:49:28 (CEST), Stefano Sabatini wrote:
> >>
> >> > On date Saturday 2010-05-15 17:06:02 +0200, Reinhard Tartler encoded:
> >> >> On Fri, May 14, 2010 at 09:16:28 (CEST), Reinhard Tartler wrote:
> >> >>
> >> >> >> its probably a good idea but the functions need to be documented in
> >> >> >> teh header
> >> >> >
> >> >> > In this case I propose the following patch for swscale:
> >> >>
> >> >> Updated Patch with improved documentation by Attila:
> >> >>
> >> >> Index: swscale.c
> >> >> ===================================================================
> >> >> --- swscale.c (revision 31179)
> >> >> +++ swscale.c (working copy)
> >> >> @@ -1424,12 +1424,12 @@
> >> >>
> >> >> if (usePal(srcFormat)) {
> >> >> switch (dstFormat) {
> >> >> - case PIX_FMT_RGB32 : conv = palette8topacked32; break;
> >> >> - case PIX_FMT_BGR32 : conv = palette8topacked32; break;
> >> >> - case PIX_FMT_BGR32_1: conv = palette8topacked32; break;
> >> >> - case PIX_FMT_RGB32_1: conv = palette8topacked32; break;
> >> >> - case PIX_FMT_RGB24 : conv = palette8topacked24; break;
> >> >> - case PIX_FMT_BGR24 : conv = palette8topacked24; break;
> >> >> + case PIX_FMT_RGB32 : conv = sws_palette8topacked32; break;
> >> >> + case PIX_FMT_BGR32 : conv = sws_palette8topacked32; break;
> >> >> + case PIX_FMT_BGR32_1: conv = sws_palette8topacked32; break;
> >> >> + case PIX_FMT_RGB32_1: conv = sws_palette8topacked32; break;
> >> >> + case PIX_FMT_RGB24 : conv = sws_palette8topacked24; break;
> >> >> + case PIX_FMT_BGR24 : conv = sws_palette8topacked24; break;
> >> >> }
> >> >> }
> >> >>
> >> >> @@ -1962,3 +1962,70 @@
> >> >> return sws_scale(c, src, srcStride, srcSliceY, srcSliceH, dst, dstStride);
> >> >> }
> >> >> #endif
> >> >> +
> >> >> +/**
> >> >> + * Convert the palette to the same packet 32-bit format as the palette
> >> >> + */
> >> >
> >> > What's the point of duplicating docs in the implementation?
> >>
> >> Perhaps Attila can comment on this better than me since he suggested
> >> that. The comment is not really duplicated, this comment is about the
> >> implementation, the documentation in the header is for users.
> >>
> >> >> +void sws_palette8topacked32(const uint8_t *src, uint8_t *dst, long num_pixels, const uint8_t *palette)
> >> >> +{
> >> >> + long i;
> >> >> +
> >> >> + for (i=0; i<num_pixels; i++)
> >> >> + ((uint32_t *) dst)[i] = ((const uint32_t *) palette)[src[i]];
> >> >> +}
> >> > [...]
> >> >> Index: swscale.h
> >> >> ===================================================================
> >> >> --- swscale.h (revision 31179)
> >> >> +++ swscale.h (working copy)
> >> >> @@ -302,4 +302,58 @@
> >> >> int flags, SwsFilter *srcFilter,
> >> >> SwsFilter *dstFilter, const double *param);
> >> >>
> >> >> +/**
> >> >> + * Convert an 8bit paletted frame into an RGB32 frame
> >> >> + * @param src: source frame buffer
> >> >> + * @param dst: destination frame buffer
> >> >> + * @param num_pixels: number of pixels to convert
> >> >> + * @param palette: array, [256] of RGB32 entries
> >> >> + */
> >> >> +void sws_palette8topacked32(const uint8_t *src, uint8_t *dst, long num_pixels, const uint8_t *palette);
> >> >
> >> > Convert -> ConvertS, please use third person. Also add an empty line
> >> > before the first @param, the ":" at the end of the param name is
> >> > inconsistent with the FFmpeg doxies.
> >> >
> >> > Same comments apply to the other doxies in the patch.
> >>
> >> changed
> >>
> >> > [...]
> >> >> #endif /* SWSCALE_SWSCALE_H */
> >> >> Index: rgb2rgb.c
> >> >> ===================================================================
> >> >> --- rgb2rgb.c (revision 31179)
> >> >> +++ rgb2rgb.c (working copy)
> >> >> @@ -207,63 +207,34 @@
> >> >> rgb2rgb_init_C();
> >> >> }
> >> >>
> >> >> -/**
> >> >> - * Convert the palette to the same packet 32-bit format as the palette
> >> >> - */
> >> >> void palette8topacked32(const uint8_t *src, uint8_t *dst, long num_pixels, const uint8_t *palette)
> >> >> {
> >> >> - long i;
> >> >> -
> >> >> - for (i=0; i<num_pixels; i++)
> >> >> - ((uint32_t *) dst)[i] = ((const uint32_t *) palette)[src[i]];
> >> >> + sws_palette8topacked32(src, dst, num_pixels, palette);
> >> >> }
> >> >
> >> > Why not directly use sws_palette8XXX() in the code, and get rid of
> >> > these senseless wrapper functions?
> >> >
> >> > They've never been public, so it should be safe to just remove
> >> > them. If you want to keep them for the sake of misbehaving apps using
> >> > the libav* internal API (hi MPlayer!) please mark them with:
> >> > #if LIBSWCALE_VERSION_MAJOR < 1
> >> > ...
> >> > #endif
> >>
> >> good idea, implemented.
> >>
> >> > But I don't think this is going to be any good, applications using
> >> > internal API are doomed to be broken anyway, if not here somewhere
> >> > else (see my eval.c patches).
> >>
> >> Yes, and I think this is undesireable. I can tell you more about the
> >> breakage as soon as I'll upload an ffmpeg 0.6 package for ubuntu/debian.
> >>
> >> >
> >> >> void rgb32to24(const uint8_t *src, uint8_t *dst, long src_size)
> >> >> Index: rgb2rgb.h
> >> >> ===================================================================
> >> >> --- rgb2rgb.h (revision 31179)
> >> >> +++ rgb2rgb.h (working copy)
> >> >> @@ -4,7 +4,7 @@
> >> >> * Software YUV to YUV converter
> >> >> * Software YUV to RGB converter
> >> >> * Written by Nick Kurshev.
> >> >> - * palette & YUV & runtime CPU stuff by Michael (michaelni at gmx.at)
> >> >> + * YUV & runtime CPU stuff by Michael (michaelni at gmx.at)
> >> >> *
> >> >> * This file is part of FFmpeg.
> >> >> *
> >> >> @@ -66,6 +66,7 @@
> >> >> void shuffle_bytes_3012(const uint8_t *src, uint8_t *dst, long src_size);
> >> >> void shuffle_bytes_3210(const uint8_t *src, uint8_t *dst, long src_size);
> >> >>
> >> >> +/* deprecated, use the public versions in swscale.h */
> >> >> void palette8topacked32(const uint8_t *src, uint8_t *dst, long num_pixels, const uint8_t *palette);
> >> >> void palette8topacked24(const uint8_t *src, uint8_t *dst, long num_pixels, const uint8_t *palette);
> >> >> void palette8torgb16(const uint8_t *src, uint8_t *dst, long num_pixels, const uint8_t *palette);
> >> >
> >> > Regards.
> >>
> >> Index: swscale.c
> >> ===================================================================
> >> --- swscale.c (revision 31179)
> >> +++ swscale.c (working copy)
> >> @@ -1424,12 +1424,12 @@
> >>
> >> if (usePal(srcFormat)) {
> >> switch (dstFormat) {
> >> - case PIX_FMT_RGB32 : conv = palette8topacked32; break;
> >> - case PIX_FMT_BGR32 : conv = palette8topacked32; break;
> >> - case PIX_FMT_BGR32_1: conv = palette8topacked32; break;
> >> - case PIX_FMT_RGB32_1: conv = palette8topacked32; break;
> >> - case PIX_FMT_RGB24 : conv = palette8topacked24; break;
> >> - case PIX_FMT_BGR24 : conv = palette8topacked24; break;
> >> + case PIX_FMT_RGB32 : conv = sws_palette8topacked32; break;
> >> + case PIX_FMT_BGR32 : conv = sws_palette8topacked32; break;
> >> + case PIX_FMT_BGR32_1: conv = sws_palette8topacked32; break;
> >> + case PIX_FMT_RGB32_1: conv = sws_palette8topacked32; break;
> >> + case PIX_FMT_RGB24 : conv = sws_palette8topacked24; break;
> >> + case PIX_FMT_BGR24 : conv = sws_palette8topacked24; break;
> >> }
> >> }
> >>
> >
> >> @@ -1962,3 +1962,70 @@
> >> return sws_scale(c, src, srcStride, srcSliceY, srcSliceH, dst, dstStride);
> >> }
> >> #endif
> >> +
> >> +/**
> >> + * Convert the palette to the same packet 32-bit format as the palette
> >> + */
> >
> > is this an english sentance?
>
> descriptions improved.
>
> >
> >> +void sws_palette8topacked32(const uint8_t *src, uint8_t *dst, long num_pixels, const uint8_t *palette)
> >> +{
> >> + long i;
> >> +
> >> + for (i=0; i<num_pixels; i++)
> >> + ((uint32_t *) dst)[i] = ((const uint32_t *) palette)[src[i]];
> >> +}
> >> +
> >> +/**
> >> + * Palette format: ABCD -> dst format: ABC
> >> + */
> >> +void sws_palette8topacked24(const uint8_t *src, uint8_t *dst, long num_pixels, const uint8_t *palette)
> >> +{
> >> + long i;
> >> +
> >> + for (i=0; i<num_pixels; i++) {
> >> + //FIXME slow?
> >> + dst[0]= palette[src[i]*4+0];
> >> + dst[1]= palette[src[i]*4+1];
> >> + dst[2]= palette[src[i]*4+2];
> >> + dst+= 3;
> >> + }
> >> +}
> >> +
> >> +/**
> >> + * Palette is assumed to contain BGR16
> >> + */
> >> +void sws_palette8torgb16(const uint8_t *src, uint8_t *dst, long num_pixels, const uint8_t *palette)
> >> +{
> >> + long i;
> >> + for (i=0; i<num_pixels; i++)
> >> + ((uint16_t *)dst)[i] = ((const uint16_t *)palette)[src[i]];
> >> +}
> >> +
> >> +/**
> >> + * Palette is assumed to contain BGR16
> >> + */
> >> +void sws_palette8tobgr16(const uint8_t *src, uint8_t *dst, long num_pixels, const uint8_t *palette)
> >> +{
> >> + long i;
> >> + for (i=0; i<num_pixels; i++)
> >> + ((uint16_t *)dst)[i] = bswap_16(((const uint16_t *)palette)[src[i]]);
> >> +}
> >> +
> >> +/**
> >> + * Palette is assumed to contain RGB15
> >> + */
> >> +void sws_palette8torgb15(const uint8_t *src, uint8_t *dst, long num_pixels, const uint8_t *palette)
> >> +{
> >> + long i;
> >> + for (i=0; i<num_pixels; i++)
> >> + ((uint16_t *)dst)[i] = ((const uint16_t *)palette)[src[i]];
> >> +}
> >> +
> >> +/**
> >> + * Palette is assumed to contain BGR15
> >> + */
> >> +void sws_palette8tobgr15(const uint8_t *src, uint8_t *dst, long num_pixels, const uint8_t *palette)
> >> +{
> >> + long i;
> >> + for (i=0; i<num_pixels; i++)
> >> + ((uint16_t *)dst)[i] = bswap_16(((const uint16_t *)palette)[src[i]]);
> >> +}
> >
> >> Index: swscale.h
> >> ===================================================================
> >> --- swscale.h (revision 31179)
> >> +++ swscale.h (working copy)
> >> @@ -302,4 +302,64 @@
> >> int flags, SwsFilter *srcFilter,
> >> SwsFilter *dstFilter, const double *param);
> >>
> >> +/**
> >> + * Converts an 8bit paletted frame into an RGB32 frame
> >
> > thats not complete, BGR and others can use this too
>
> improved as well.
>
> >
> >> + *
> >> + * @param src source frame buffer
> >> + * @param dst destination frame buffer
> >> + * @param num_pixels number of pixels to convert
> >> + * @param palette array, [256] of RGB32 entries
> >> + */
> >> +void sws_palette8topacked32(const uint8_t *src, uint8_t *dst, long num_pixels, const uint8_t *palette);
> >> +
> >> +/**
> >> + * Converts an 8bit paletted frame into an RGB24 frame
> >
> > same
> >
> >
> >> + *
> >> + * @param src source frame buffer
> >> + * @param dst destination frame buffer
> >> + * @param num_pixels number of pixels to convert
> >
> >> + * @param palette array, [256] of RGB24 entries
> >
> > it should be const uint8_t palette[256] in the code then
>
> I'd prefer to defer this for a later patch, as this is IMO a different
> change than this refactoring. please say if you disagree.
>
> >
> > [...]
> >> +/* deprecated, use the public versions in swscale.h */
> >> void palette8topacked32(const uint8_t *src, uint8_t *dst, long num_pixels, const uint8_t *palette);
> >> void palette8topacked24(const uint8_t *src, uint8_t *dst, long num_pixels, const uint8_t *palette);
> >> void palette8torgb16(const uint8_t *src, uint8_t *dst, long num_pixels, const uint8_t *palette);
> >
> > there is a attribute for deprecation and we have some wraper macro for
> > non gcc compatibility.
>
> okay, used attribute_deprecated then.
>
>
> BTW, during my work I noticed that palette8torgb16 and palette8torgb15
> (and their bgr counterparts) implement exactly the same code. Is this
> really intended?
certainly not
this should definitly be fixed before making them officially public
>
> Index: swscale.c
> ===================================================================
> --- swscale.c (revision 31179)
> +++ swscale.c (working copy)
> @@ -1424,12 +1424,12 @@
>
> if (usePal(srcFormat)) {
> switch (dstFormat) {
> - case PIX_FMT_RGB32 : conv = palette8topacked32; break;
> - case PIX_FMT_BGR32 : conv = palette8topacked32; break;
> - case PIX_FMT_BGR32_1: conv = palette8topacked32; break;
> - case PIX_FMT_RGB32_1: conv = palette8topacked32; break;
> - case PIX_FMT_RGB24 : conv = palette8topacked24; break;
> - case PIX_FMT_BGR24 : conv = palette8topacked24; break;
> + case PIX_FMT_RGB32 : conv = sws_palette8topacked32; break;
> + case PIX_FMT_BGR32 : conv = sws_palette8topacked32; break;
> + case PIX_FMT_BGR32_1: conv = sws_palette8topacked32; break;
> + case PIX_FMT_RGB32_1: conv = sws_palette8topacked32; break;
> + case PIX_FMT_RGB24 : conv = sws_palette8topacked24; break;
> + case PIX_FMT_BGR24 : conv = sws_palette8topacked24; break;
> }
> }
>
> @@ -1962,3 +1962,58 @@
> return sws_scale(c, src, srcStride, srcSliceY, srcSliceH, dst, dstStride);
> }
> #endif
> +
> +/* Convert the palette to the same packed 32-bit format as the palette */
"convert a to the same format as a"
is a circular reference
> +void sws_palette8topacked32(const uint8_t *src, uint8_t *dst, long num_pixels, const uint8_t *palette)
> +{
> + long i;
> +
> + for (i=0; i<num_pixels; i++)
> + ((uint32_t *) dst)[i] = ((const uint32_t *) palette)[src[i]];
> +}
> +
> +/* Palette format: ABCD -> dst format: ABC */
> +void sws_palette8topacked24(const uint8_t *src, uint8_t *dst, long num_pixels, const uint8_t *palette)
> +{
> + long i;
> +
> + for (i=0; i<num_pixels; i++) {
> + //FIXME slow?
> + dst[0]= palette[src[i]*4+0];
> + dst[1]= palette[src[i]*4+1];
> + dst[2]= palette[src[i]*4+2];
> + dst+= 3;
> + }
> +}
> +
> +/* Palette is assumed to contain RBG16 */
rbg?
> +void sws_palette8torgb16(const uint8_t *src, uint8_t *dst, long num_pixels, const uint8_t *palette)
> +{
> + long i;
> + for (i=0; i<num_pixels; i++)
> + ((uint16_t *)dst)[i] = ((const uint16_t *)palette)[src[i]];
> +}
> +
> +/* Palette is assumed to contain BGR16 */
> +void sws_palette8tobgr16(const uint8_t *src, uint8_t *dst, long num_pixels, const uint8_t *palette)
> +{
> + long i;
> + for (i=0; i<num_pixels; i++)
> + ((uint16_t *)dst)[i] = bswap_16(((const uint16_t *)palette)[src[i]]);
the bswap can be merged into the palette
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100516/8cd4ca5b/attachment.pgp>
More information about the ffmpeg-devel
mailing list