[MPlayer-dev-eng] [PATCH] Audio support for AVISynth

[Reimar Doeffinger]

Hello,
On Mon, Jul 24, 2006 at 01:24:34AM +0200, Gianluigi Tiesi wrote:
> Ok, even is funny that I cannot make cosmetics changes to my code,

You can. But you can't expect me to review it. I feel that my time has
at least some value.

> I've removed all cosmetics, also changed a bit the patch:

Thanks. Though I have to ask: do actually read the patches you submit,
because I still see some obvious cosmetics. If you don't, do it and
maybe you understand what my problem with cosmetic chnages is.

> -/* Implement RGB MODES ?? */
> +/* TODO: Implement all color spaces */

has nothing to do with audio support. Feel free to submit in a seperate
cosmetics-only patch.

> +        sh_video_t *sh_video = ((demux_stream_t *) ds)->sh;
> +        if (AVS->video_info->num_frames < AVS->frameno) return 0; // EOF
> +    
> +        curr_frame = AVS->avs_get_frame(AVS->clip, AVS->frameno);
> +        if (!curr_frame)
> +        {
> +            mp_msg(MSGT_DEMUX, MSGL_V, "AVS: error getting frame -- EOF ??\n");
> +            return 0;
> +        }

You only added a few spaces here, please, if you move it to a seperate
patch everyone will have a much easier time to understand what this
patch actually does, what it changes and why it didn't work before (or
in other words, learn from it).

> -        d_video->pts=AVS->frameno / sh_video->fps; // OSD
> +        dp->pts = (float) AVS->frameno / sh_video->fps;

someone else already mentioned: (double) would be better than (float)
(the fps variable might be better to make double, too, just to be safe).
Though you shouldn't need to use a cast here at all.
Also it seems to me that this is a bug independant from the audio
support? I'd apply it seperately before anything else, or does something
speak against that?

> +        AVS->frameno++;
> +        AVS->avs_release_video_frame(curr_frame);

Why were these two lines moved up? Is this necessary for the audio
support to work or is this a seperate issue or just "cosmetic"?

> +        dp->pts = (double) AVS->sampleno / (double) sh_audio->wf->nSamplesPerSec;

hehe, here actually double is already used *g*

> +    AVS->fps = (float) ((float) AVS->video_info->fps_numerator / (float) AVS->video_info->fps_denominator);

This is where I meant double might be better to be on the safe side. I
think float had some problems for framerates like 30000/1001. Also,
three casts is a bit overkill, just 
AVS->fps = (double)AVS->video_info->fps_numerator / (double)AVS->video_info->fps_denominator;
should be more than enough.
Though I think it might be preferable to put this in a seperate
"fix/improve pts/fps handling" patch.

> +        /* Only YV12 supported for now */
unrelated and cosmetic
[...]
> -    // TODO release_clip?
> +
[...]
> +            if (AVS->clip) AVS->avs_release_clip(AVS->clip);

seems unrelated to me -> seperate patch?

> -    //mp_msg(MSGT_DEMUX, MSGL_V, "AVS: seek rel_seek_secs = %f - flags = %x\n", rel_seek_secs, flags);
> -    

unrelated and cosmetic?

> +        AVS->sampleno = (int) floor((float) AVS->frameno / sh_video->fps * (float) sh_audio->wf->nSamplesPerSec);

Any reason not to use AVS->fps here? esp. since I'm not sure if
sh_video->fps doesn't get changed when you use -speed or -fps command
line options, that might have weird effects on that code then.

Greetings,
Reimar Doeffinger