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

[Uoti Urpala]

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.

> 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. 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).

> Apart from that I am not at all convinced that all the demuxer-specific
> code in video.c should be there and not at least partially in the
> demuxers.
> A large part of it I think could be removed by just making the demuxers
> set sh->format correctly and introducing a needs_parsing variable (for
> what I can tell find_video_codec does not really find the video codec,
> it finds how if at all the stream needs to be parsed into packets).
> Particularly the 3 ES demuxer variants look quite idiotic to me, here
> the demuxer format seems to have been misused to identify the codec -
> for which we have sh->format after all.

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.

Note that the video_read_frame() code is already ignored with
correct-pts, only the video_read_properties() part still matters then.