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

[Gianluigi Tiesi]

On Tue, Jul 25, 2006 at 03:45:43PM +0200, Reimar Doeffinger wrote:
> 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.
I prefer to look at overall resulting file instead of looking
at the patch.
> 
> > -/* 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.
yes, maybe
> 
> > +        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).
as Alexander replied its another block of code, also this is an example
where looking at the patch is not better than looking at the resulting
file.
> 
> > -        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?
not my code, I used double, I only adapted Alexander patch the reduce the
patch size
> 
> > +        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"?
I cannot spend hours to force minimize diff :P
> 
> > +        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.
Agree
> 
> > +        /* 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?
hmm yes but are you sure you want 240 patches 1 line each one?
> 
> > +        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.
not my code here you must ask to Alexander
> 

Regards

-- 
Gianluigi Tiesi <sherpya at netfarm.it>
EDP Project Leader
Netfarm S.r.l. - http://www.netfarm.it/
Free Software: http://oss.netfarm.it/