[FFmpeg-devel] lavfi noise generator

[Michael Niedermayer]

On Wed, Jan 14, 2009 at 01:34:41AM +0100, Stefano Sabatini wrote:
> On date Saturday 2009-01-03 18:09:49 +0100, Stefano Sabatini encoded:
> > On date Monday 2008-12-29 13:45:23 +0100, Michael Niedermayer encoded:
> > > On Mon, Dec 29, 2008 at 11:39:30AM +0100, Stefano Sabatini wrote:
> > > > On date Monday 2008-12-29 00:52:18 +0100, Michael Niedermayer encoded:
> > > > > On Mon, Dec 29, 2008 at 12:25:02AM +0100, Stefano Sabatini wrote:
> > > > > > Hi all,
> > > > > > 
> > > > > > no Michael I'm not posting it for you or others to review it, I'm just
> > > > > 
> > > > > no but i reject it anyway
> > > > > your code is crap, sorry
> > > > > 
> > > > > People didnt want to port libmpcodecs when i suggested it, i dont mind
> > > > > this at all, libmpcodecs has its serious issues but this does not mean i
> > > > > will accept code that is inferrior than it.
> > > > > 
> > > > > vf_noise.c should be MUCH faster than your code.
> > > [...]
> > > > As for the speed concerns, I see two major issues to be addressed, the
> > > > use of the av_random() for setting every single byte, ideally we
> > > > should set the whole buffer for example reading from /dev/urandom but
> > > > this would not be portable, and the av_picture_copy() can be avoided
> > > > using properly the permissions system and passing a picture reference.
> > > 
> > > did you read vf_noise.c ?
> > 
> > What about a noise module in libavutil (based on your vf_noise.c)?
> > 
> > For example it may be shared between a noise source and filter, also I
> > think those routines may be useful also in other places.
> 
> In attachment my idea, yes it still needs some rework (mainly I need
> to understand better the code of noise.[ch]), I basically just did
> some monkey translation from your code.

[...]

> typedef struct AVNoiseContext {

>     AVRandomState rand_state;   ///< random state used to generate the initial random noise

why do you use MT instead of the simpler generators?


>     unsigned int seed;          ///< seed used by \p rand_state
>     int strength;
>     int flags;                  ///< NOISE_FLAG_* flags controlling how the noise will be generated
>     int shiftptr;
>     int8_t *noise;
>     unsigned int noise_size;
>     int8_t *prev_shift[MAX_RES][3];
> } AVNoiseContext;
> 
> /**
>  * Allocate and initialize a buffer containing noise.
>  *
>  * @param ctx the noise context containing the parameter which will
>  * control how the noise is generated
>  * @return 0 if successful, a negative value otherwise
>  */
> int av_noise_init(AVNoiseContext *ctx);

Should this really be part of the public api from the begin?
this will need more time to make sure the API is optimal

also all this is no patch ...


[...]

> #define RAND_N(strenght) ((int) ((double)strenght * av_random(&ctx->rand_state) / (RAND_MAX + 1.0)))

RAND_MAX ?
our MT code was merged into iso C ?


[...]
>     noise= av_malloc(noise_size * sizeof(int8_t));
>     if (!noise)
>         return -1;

sizeof(int8_t) is not usefull



> 
>     av_init_random(ctx->seed, &ctx->rand_state);
> 
>     for(i=0, j=0; i < noise_size; i++, j++) {
>         if(ctx->flags & NOISE_FLAG_UNIFORM) {
>             if (ctx->flags & NOISE_FLAG_AVERAGED) {
>                 if (ctx->flags & NOISE_FLAG_PATTERN) {
>                     noise[i]= (RAND_N(strength) - strength/2)/6
>                         +patt[j%4] * strength * 0.25/3;
>                 } else {
>                     noise[i]= (RAND_N(strength) - strength/2)/3;
>                 }
>             } else {
>                 if (ctx->flags & NOISE_FLAG_PATTERN) {
>                     noise[i]= (RAND_N(strength) - strength/2)/2
>                         + patt[j%4] * strength * 0.25;
>                 } else {
>                     noise[i]= RAND_N(strength) - strength/2;
>                 }
>             }
>         } else {

>             double x1, x2, w, y1;
>             do {
>                 x1 = 2.0 * av_random(&ctx->rand_state)/(float)RAND_MAX - 1.0;
>                 x2 = 2.0 * av_random(&ctx->rand_state)/(float)RAND_MAX - 1.0;
>                 w = x1 * x1 + x2 * x2;
>             } while ( w >= 1.0 );
> 
>             w = sqrt( ( -2.0 * log(w) ) / w );

testing the binary difference between float implementations? ;)


[...]
> inline void av_noise_fill(AVNoiseContext *ctx, uint8_t *dst, unsigned int buf_size, int shift)
> {
>     int i;
>     for(i=0; i < buf_size; i++)
>         dst[i] = ctx->noise[(i+shift) % ctx->noise_size];

modulo in the innermost loop? why dont you just calc sha1 per bit ;)
sorry but this "patch" is poor quality

ahh and non static inline ...

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I am the wisest man alive, for I know one thing, and that is that I know
nothing. -- Socrates
-------------- 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/20090124/ead812a3/attachment.pgp>