[FFmpeg-devel] make swscale's palette functions public

Reinhard Tartler siretart
Sun May 16 22:32:38 CEST 2010


On Sun, May 16, 2010 at 20:58:36 (CEST), Michael Niedermayer wrote:

> 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

AFAIUI, you have written these functions. Could you perhaps take a look
at them?

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

This is just a moving around of a comment of the previous author (i.e.,
you). What would be the correct comment here? Or should it be dropped?

>
>> +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?

again, I also guess you meant RGB here.

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

I'm not sure what you mean here, how can this be done without breaking
applications that already use this function (e.g. mplayer)?


-- 
Gruesse/greetings,
Reinhard Tartler, KeyID 945348A4




More information about the ffmpeg-devel mailing list