[MPlayer-dev-eng] Re: [PATCH] demux_avi: instant A-V sync correction on seeking

[Ivan Kalvachev]

2006/2/7, Corey Hickey <bugfood-ml at fatooh.org>:
> Corey Hickey wrote:
> >>>>The attached patch makes demux_avi recognize the current specified delay
> >>>>and adjust the requisite stream position accordingly. On the first test
> >>>>case above, mplayer will behave the same initially, but seeking will
> >>>>instantly delay the video 10 seconds. Seeking after adjusting sync using
> >>>>the +/- keys will work just as well. Stream delays from dwStart will
> >>>>work well, too, once dwStart is supported.
> >>>
> >>>Hmm... no responses. If I don't hear any negative feedback in 24 hours,
> >>>I'll assume this patch isn't an atrocity and I will commit it.
> >>
> >>
> >>I must say i really don't like the direct import of audio_delay in the
> >>demuxer layer. imho it would be better to pass it via arguments or
> >>at worst via some control (called right before seek).
> >
> >
> > Ok. I had used it as an external variable because it already was in
> > mplayer.c. This patch changes it to an argument to demux_seek().
> >
> > I'll apply this after 48 hours if I don't get any feedback at all. This
> > fixes the only objection so far, but it is the weekend.... Somebody say
> > "apply it now" if the patch looks good.
>
> Applied. I only waited 45 hours. Forgive me; I thought the deadline had
> passed until I re-checked when I sent the parent message after already
> committing.

Would You revert it. It seem that there are some problems and even
users seek instructions how to retrive older version.


I would give you some of the commend I saw in #mplayerdev

Feb 08 11:40:53 <uau>   seems that the seek audio_delay change was
quite broken - the delay is missing from the demuxer->desc->seek call
in demuxer.c/demux_seek
Feb 08 11:41:41 <uau>   since some changes were committed to that file
it's not just that it was missing from the commit - i
wonder how it was tested, it seems that it could not have done any
good as it is now...
Feb 08 11:56:18 <uau>   hmm the audio delay seek patch is even more broken
Feb 08 11:56:29 <dalias>        uau, :(
Feb 08 11:56:44 <ods15> hey i just noticed, did he break my demux_nut :)
Feb 08 11:56:45 <uau>   it changes some of the demux_XXX_seek
functions to have the delay parameter, but not all
Feb 08 11:56:55 <uau>   but of course they should have a common prototype...
Feb 08 11:57:05 <ods15> uau: yeah that can crash...
Feb 08 11:57:28 <dalias>        uau, can you fix it?
Feb 08 11:58:09 <uau>   dalias: maybe i could but it'd take several changes...
Feb 08 12:00:45 <uau>   so what the patch does now is 1) give the
delay parameter from mplayer.c to demuxer.c, which however
ignores it and calls container-specific seek functions without it
Feb 08 12:01:30 <uau>   and 2) change SOME of the container-specific
seek functions (which should share a common prototype) to take a delay
parameter (their common prototype however does not have that
parameter, and they're called without it)
Feb 08 12:02:54 <uau>   as it's that broken and the brokenness doesn't
look like it could have been caused by just failing to commit some
files, i doubt the patch has really been tested at all (i can't see
how it could do any good in any situation like it is now)
Feb 08 12:03:58 <iive>  uau, afaik this patch was discussed for weeks in dev-eng
Feb 08 12:05:34 <dalias>        i expect just some files were missed
in the commit..
Feb 08 12:05:36 <uau>   iive: was it that patch?
Feb 08 12:05:59 <uau>   some decode delay related stuff was discussed,
but was this audio sync delay patch discussed that much?
Feb 08 12:06:29 <ods15> just a single mail from albeu
Feb 08 12:06:42 <ods15> (and 2 mails from corey)
Feb 08 12:06:44 <ods15> bbl
Feb 08 12:06:51 <uau>   dalias: demuxer.c had SOME changes, but was wrong
Feb 08 12:07:01 <uau>   so it can't be just missing files from commit
Feb 08 12:07:43 <uau>   demuxer.c/demux_seek() was changed to take an
audio_delay parameter, but was not changed to give that parameter to
the container-specific seek functions
Feb 08 12:11:27 <uau>   reading the mails related to that patch, it
seems that the original version used the global audio_delay variable
directly in the avi demuxer
Feb 08 12:11:45 <uau>   Alban Bedel sent a mail objecting to that,
saying it should be a parameter instead
Feb 08 12:12:16 <uau>   Corey changed it to be passed as a parameter
instead, breaking stuff badly
Feb 08 12:12:34 <uau>   nobody commented on that patch, and he
eventually committed it (apparently without any testing...)
Feb 08 12:17:56 <dalias>        well can you send an email with these findings
Feb 08 12:17:59 <dalias>        and some cola for corey?

I have removed some of the non-related messages.