[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