[FFmpeg-devel] [RFC] Eliminate -re and make -rate_emu set by AVOption

[Stefano Sabatini]

On date Wednesday 2008-07-16 12:50:10 -0700, Baptiste Coudurier encoded:
> Hi Luca,
> 
> Luca Abeni wrote:
> > Hi Stefano,
> > 
> > sorry for the late reply, but I wanted to triple-think about this issue 
> > before writing something...
> > 
> > Stefano Sabatini wrote:
> > [...]
> >> My solution is to make -rate_emu settable through AVOption options
> >> using rate_emu, then remove the -re option.
> >>
> >> This alone doesn't fix the problems for audio-only multimedia streams
> >> streaming, there are still timestamp issues to be addressed.
> > 
> > If I understand your patch correctly, it allows to set the rate_emu 
> > field in the audio codec context, but it does not provide support for 
> > "audio frame rate emulation". In fact, I believe that the following code 
> > can only work for the video streams:
> >>         /* frame rate emulation */
> >>         if (ist->st->codec->rate_emu) {
> >>             int64_t pts = av_rescale((int64_t) ist->frame * ist->st->codec->time_base.num, 1000000, ist->st->codec->time_base.den);
> >>             int64_t now = av_gettime() - ist->start;
> >>             if (pts > now)
> >>                 usleep(pts - now);
> >>
> >>             ist->frame++;
> >>         }
> > (I do not believe that an audio pts can be computed as frame number * 
> > time base).

Yes, indeed as I already wrote currently -re works properly only with
video streams where is:
time_base = 1 /frame_rate

> > So, I suspect that setting rate_emu for audio codecs is quite pointless...
> > (In other words: if we really want to go this path, I believe we should 
> > first fix audio frame rate emulation, and then enable it in a second time)
> > 
> > Moreover, why does the rate emulation code cited above compute a pts in 
> > that way? Cannot ist->next_pts (or something similar) be used?
> 
> I think this would be more correct indeed.

Yes, and that's true maybe we should first fix the PTS issues, and
only *then* apply this patch, on the other hand this patch makes
easier to test the modifications for audio streams, currently not
supported at all.

> > I also have a second (bigger) doubt: in my opinion, rate_emu should not 
> > be a field in AVCodecContext. In fact, nothing else than ffmpeg.c uses 
> > it (unless I misread some code). So, I would be tempted to remove this 
> > field from AVCodecContext, and to use the global rate_emu variable in 
> > the "if()" quoted above (so, I would change "if 
> > (ist->st->codec->rate_emu) {" in "if (rate_emu) {").
> 
> Yes, I tend to agree with this.

Indeed currently rate_emu is special since is a field which is *never*
used by the library, but only by ffmpeg/other applications.

Nonetheless I think it is quite useful to keep it as an AVCodecContext
field, see the nice property of the patch of which I already wrote,
-[avs]rate_emu 1 will activate the frame_rate emulation mechanism only
for the corresponding stream, this with just one global shouldn't be
possible. Also it doesn't appear to me such a crazy thing the idea to
implement the rate_emu mechanism directly in the library (the decoding
function could usleep up to when it will be time to output the decoded
frame, as currently implemented in ffmpeg -re).

What's the opinion of the maintainers?

Regards.
-- 
FFmpeg = Fostering & Funny Magic Programmable Educated God