[Ffmpeg-devel] swscale patch

[Luca Abeni]

Hi Michael,

On Fri, 2006-06-30 at 21:05 +0200, Michael Niedermayer wrote:
[...]
> > Index: swscaler_glue.c
> > ===================================================================
> > --- swscaler_glue.c	(revision 0)
> > +++ swscaler_glue.c	(revision 0)
> > @@ -0,0 +1,53 @@
> > +#include "config.h"
> > +#include "swsutil.h"
> > +#include "swscale.h"
> > +#include "img_format.h"
> > +
> > +void *(*sws_malloc)(unsigned int size);
> > +void (*sws_free)(void *ptr);
> > +void (*sws_log)(void*, int level, const char *fmt, ...);
> > +
> > +void sws_global_init(void *(*alloc)(unsigned int size),
> > +        void (*free)(void *ptr), void (*log)(void*, int level, const char *fmt, ...))
> > +{
> > +    sws_malloc = alloc;
> > +    sws_free = free;
> > +    sws_log = log;
> > +}
> 
> hmm, this is called just from one spot, in sws_init() is there any sense in
> this double layer init?
It is probably overkilling... My idea was: sws_global_init() is part of
the swscale interface, whule sws_init() (bad name, probably) is an
mplayer/mencoder function used to avoid duplicated code.
(I introduced it to avoid duplicating the code contained in
sws_interface.c)
But after all sws_interface.c is just few lines of code. If you want, I
can copy that code in mplayer.c and mencoder.c, eliminating sws_init().

> > +
> > +/* Used for ffmpeg --> MPlayer format name conversion */
> > +static int fmt_name[PIX_FMT_NB] = {
> 
> static -> static const
Ok, I'll fix it.

[...]
> > +    [PIX_FMT_RGB555] = IMGFMT_RGB15,    ///< always stored in cpu endianness, most significant bit to 1 
> > +    [PIX_FMT_UYVY422] = IMGFMT_UYVY,   ///< Packed pixel, Cb Y0 Cr Y1 
> > +};
> > +
> > +int sws_pixel_format(enum PixelFormat fmt)
> 
> hmm not so good name, what about sws_ff2mp_pixfmt() or a longer one
Ok, it'll become sws_ff2mp_pixfmt()


> [...]
> > +char *sws_format_name(int format)
> > +{
> > +    static char fmt_name[64];
> > +    char *res;
> > +    static int buffer;
> > +
> > +    res = fmt_name + buffer * 32;
> > +    buffer = 1 - buffer;
> > +    snprintf(res, 32, "0x%x (%c%c%c%c)", format,
> > +		    format >> 24, (format >> 16) & 0xFF,
> > +		    (format >> 8) & 0xFF,
> > +		    format & 0xFF);
> > +
> 
> this isnt thread safe
Opss... I did not think about threads. I see two ways to fix it:
1) using a table of strings, or a switch as in vo_format_name
2) changing the interface so that the caller provides the buffer

Since this function is not going to stay here for a long time (I plan to
switch to ffmpeg pixel format names, and then avcodec_get_pix_fmt_name()
could be used) I am tempted to just copy vo_format_name(). Which
solution do you prefer?

> [...]
> > -	if(c->yuvTable) free(c->yuvTable);
> > +	if(c->yuvTable) sws_free(c->yuvTable);
> 
> IMO sws_free(NULL) should be supported so that these checks arent needed
Since sws_free() ends up to be free(), sws_free(NULL) is ok... I copied
the code from the existing one without thinking about it. I'll remove
all those "if". Or there is some platform in which free(NULL) is not
valid? (in such case, I'll remove the "if"s and fix the problem in
sws_free).

> [...]
> > ===================================================================
> > --- swscale.h	(revision 18866)
> > +++ swscale.h	(working copy)
> > @@ -18,7 +18,7 @@
> >  
> >  #ifndef SWSCALE_H
> >  #define SWSCALE_H
> > -
> > +#include <stdint.h>
> 
> why?
Opss... Sorry. I don't know why I put it there. I am removing this include.

> [...]
> > Index: mangle.h
> > ===================================================================
> > --- mangle.h	(revision 0)
> > +++ mangle.h	(revision 0)
> > @@ -0,0 +1,19 @@
> > +/* mangle.h - This file has some CPP macros to deal with different symbol
> > + * mangling across binary formats.
> > + * (c)2002 by Felix Buenemann <atmosfear at users.sourceforge.net>
> > + * File licensed under the GPL, see http://www.fsf.org/ for more info.
>                               ^^^
> this isnt used without --enable-gpl ?
I saw it was needed by swscale, and I copied it without changing the
license (right now libswscale is compiled only if --enabled-gpl is
provided).
Anyway, I see that the MANGLE() macro is already provided by libavutil
under LGPL... I can use that one, to avoid licensing problems and
duplicate code.

> [...]
> > Index: img_format.h
> > ===================================================================
> > --- img_format.h	(revision 0)
> > +++ img_format.h	(revision 0)
> > @@ -0,0 +1,109 @@
> > +#ifndef __IMG_FORMAT_H
[...]
> > +#define IMGFMT_XVMC_IDCT_MPEG2 (IMGFMT_XVMC|0x82)
> > +
> > +#endif
> 
> just to make sure ... this is a temporary solution and not intended to stay?
Yes, this is a temporary solution. I plan to switch to ffmpeg pixel
format names. I hope this can be done in an incremental way after the
new code is in subversion

> furthermore, can this file be checked out as external instead of being
> duplicated?
Ok, I'll use an external

> [...]
> > Index: rgb2rgb.h
> > ===================================================================
> > --- rgb2rgb.h	(revision 18866)
> > +++ rgb2rgb.h	(working copy)
> > @@ -125,20 +125,6 @@
> >  			long srcStride3, long dstStride);
> >  	
> >  
> > -#define MODE_RGB  0x1
> > -#define MODE_BGR  0x2
> > -
> > -static void yuv2rgb(uint8_t * image, uint8_t * py,
> > -			      uint8_t * pu, uint8_t * pv,
> > -			      unsigned h_size, unsigned v_size,
> > -			      int rgb_stride, int y_stride, int uv_stride){
> > -printf("broken, this should use the swscaler\n");
> > -}
> > -
> > -static void yuv2rgb_init (unsigned bpp, int mode){
> > -printf("broken, this should use the swscaler\n");
> > -}
> > -
> >  void sws_rgb2rgb_init(int flags);
> >  
> >  #endif
> 
> does this depend on the rest? if no apply it!
It is independent from the rest of the patch (I am just re-testing, to
be sure). I am going to commit this part of the patch in a short time
(after some tests).

> [...]
> > Index: cpu.h
> > ===================================================================
> > --- cpu.h	(revision 0)
> > +++ cpu.h	(revision 0)
> > @@ -0,0 +1,34 @@
> > +#ifndef __SWS_CPU_H__
> > +#define __SWS_CPU_H__
> > +
> > +#ifdef ARCH_X86_64
> > +#  define REG_a "rax"
> > +#  define REG_b "rbx"
> > +#  define REG_c "rcx"
> > +#  define REG_d "rdx"
> > +#  define REG_D "rdi"
> > +#  define REG_S "rsi"
> > +#  define PTR_SIZE "8"
> > +#  define REGa    rax
> > +#  define REGb    rbx
> > +#  define REGSP   rsp
> > +#  define REG_SP "rsp"
> > +#  define REG_BP "rbp"
> > +#  define REGBP   rbp
> > +#else
> > +#  define REG_a "eax"
> > +#  define REG_b "ebx"
> > +#  define REG_c "ecx"
> > +#  define REG_d "edx"
> > +#  define REG_D "edi"
> > +#  define REG_S "esi"
> > +#  define PTR_SIZE "4"
> > +#  define REGa    eax
> > +#  define REGb    ebx
> > +#  define REGSP   esp
> > +#  define REG_SP "esp"
> > +#  define REG_BP "ebp"
> > +#  define REGBP   ebp
> > +#endif
> > +
> > +#endif	/* __SWS_CPU_H__ */
> > 
> 
> one day this needs to be cleaned up and put in a single place per project ...
I agree.
I initially saw some of these macros in libavcodec/i386/mmx.h, so I
tried to use such header. But that is the wrong header, so I ended up
creating this "cpu.h"...

Maybe we should put these macros in libavutils/x86_cpu.h (or something
similar), and include such file from all the places that use REG_*?
Let me know if you agree, and I will provide a patch for ffmpeg.


			Thanks,
				Luca