[MPlayer-cvslog] r29610 - trunk/libmpdemux/demux_demuxers.c

[Reimar Döffinger]

On Wed, Sep 23, 2009 at 12:32:44PM +0300, Uoti Urpala wrote:
> n Wed, 2009-09-23 at 09:56 +0200, Reimar Döffinger wrote:
> > On Wed, Sep 23, 2009 at 05:36:01AM +0300, Uoti Urpala wrote:
> > > On Tue, 2009-09-01 at 16:51 +0200, reimar wrote:
> > > > Hack demux_demuxers so that demux_demuxers_fill_buffer is actually called.
> > > 
> > > > Modified: trunk/libmpdemux/demux_demuxers.c
> > > > ==============================================================================
> > > > --- trunk/libmpdemux/demux_demuxers.c	Tue Sep  1 14:12:45 2009	(r29609)
> > > > +++ trunk/libmpdemux/demux_demuxers.c	Tue Sep  1 16:51:49 2009	(r29610)
> > > > @@ -54,6 +54,9 @@ demuxer_t*  new_demuxers_demuxer(demuxer
> > > >    ret->video = vd->video;
> > > >    ret->audio = ad->audio;
> > > >    ret->sub = sd->sub;
> > > > +  if (vd) vd->video->demuxer = ret;
> > > 
> > > Unfortunately this breaks all the demuxer-specific code in video.c which
> > > now sees the file type as "demux_demuxers type".
> > > 
> > > What was your motivation for this change? To enable r29611? If so then
> > > rather than changing video.c that subtitle problem is probably better
> > > fixed otherwise. I think 29611 is not a correct fix anyway - it only
> > > ensures there's one demuxed packet, but sometimes more than one would be
> > > needed.
> > 
> > The subtitle code pulls in more by itself as long as there is one.
> 
> By what mechanism? The code is pretty much designed with the goal it
> should not trigger reads from the demuxer. The only way it could pull
> more without being buggy in normal use would be if it'd somehow trigger
> the added code in demux_demuxers again (via other streams?) but I don't
> see how that'd happen either.

Ah, that you mean. I fear it does assume not more than one subtitle
packet per video packet on average.
The whole solution is rather crappy, instead the demuxer should indicate
non-interleaved streams and then the subtitle code should just go ahead
and read, but this change was a simple way to get a idea of how things
work currently and what is broken and what kinds of things should be
fixed in the long run while proving that a previously completely broken
feature is at least fixable.

> > But the principle motivation is that demux_demuxer_fill buffer should be
> > consistently called, not that sometimes it is called (well, it probably
> > was never called previously, but that was more an accident than
> > intentional), sometimes the subdemuxers directly.
> > And while the subtitles are the only case where it is actually
> > necessary, the demuxers are supposed to make sure data is adequately
> > interleaved, which is only possible if the reads go through
> > demux_demuxers.
> 
> What "adequate interleaving"? I don't see any reason why it would be
> generally _desirable_ for reading one stream to imply buffered packets
> appearing in another, so this would rather be something to avoid (when
> there's no cost to doing so), not something to make sure of.

Well, maybe, though I am not 100% sure you couldn't fiddle with mov edit
lists to get a similar issue for audio streams as for subtitle streams.
As such only the demuxer has the possibility to provide whichever
packets are available without excessive reading/seeking.
But even in the worst case at least my argument that having a fill_buffer
function that will never be called is no good remains :-)
It might be better to just get rid of it though - on the plus side this
change highlighted some issues, like the audio demuxer doing ds->demuxer
instead of using the demuxer argument directly.

> The
> subtitles are a special case because trying to read the next packet
> naively could trigger unlimited buffering of other streams. Since the
> subtitle code already has to be aware of such problems I think it would
> be logical to make it aware of this detail too, and unconditionally call
> ds_get_next_pts() if appropriate. The decision to do that could be based
> on a flag set in the demuxer/demux_stream object (probably better than
> hardcoding a check against specific demuxer types there).

Yep, basically what I meant by "indicate non-interleaved streams".
Do you still object to sh_common addition? I'd rather add that
interleaved flag there instead of on an as-needed-basis that increases
the mess.
While your argument that e.g. the pts fields do not have the same
meaning, having them in one sh_common means the documentation is in one
place and the differences become obvious (and anyway the correct
solution to the same field having different uses would be to give it
different names, not hiding the inconsistency by duplicating code).

> Yes I agree that most of the checks shouldn't be there. I didn't mean to
> imply that the current video.c behavior would be desirable, just that
> changing it (with demux_demuxers behavior) would not be a good way to
> solve the current known bugs.

I was just saying that they aren't a particularly good argument for
reverting to the old code either :-)