[gst-devel] MPEG demux restructuring

[Martin Soto]

Hello Ronald,

On Tue, 2004-03-23 at 18:18, Ronald Bultje wrote:
> Looks good, mostly. As for the timing, I noticed you've noticed the same
> oddness (scr adjustment from stream) in mpegdemux that I noticed some time
> ago. I;'ve put a patch in a low-number (#99...) bug in bugzilla, which
> removes it completely. Please use that instead. A property is not a good
> idea here, the whole scr adjustment thing is broken, imo.

Yeah, it also looked like that to me, but I didn't know if anyone was
using it. I'll get rid of that altogether.

> Some comments:
> - cur_video etc. seem to be actual realpads, not ghostpads. That's *not*
> good, imo. Make them ghostpads to their respective audio/video/priv pads
> and use those instead. 

Well, first of all, I'm keeping the normal pads, both in mpegdemux and
in dvddemux. The current_* pads are actually no ghost pads, but they
always work as a replica of one of the corresponding substream pads.
Which substream will be replicated, depends on which stream is currently
selected (see below) and can change dynamically. Actually, I didn't use
ghost pads because someone mentioned on the list that ghost pads
couldn't be dynamically moved between real pads. The other issue is that
I'd like to be able to statically connect to the current_* pads, which
is not possible with the normal stream pads. If this two things are
possible with ghost pads, I'm willing to change the code to use them,
since that would make things simpler anyway.

> Also, I'd love it if you would rename the pads
> in dvddemux to audio_%02d etc. Use GST_MPEG_DEMUX (dvddemux)->numaudiopads
> to create them. They *are* audio pads. Pad names should indicate types,
> not the caps that will be set on it. ;).

I'm not sure I understand. Both mpegdemux and dvddemux use pads named
audio_%02d. The only thing is that the audio pad template in dvddemux
has a different name, because the core doesn't allow me to override the
template defined in mpegdemux and I need to override it, since DVDs
support more audio types. I asked some time ago what I should do, but
there seems not to be an agreement about what is right to do in this
case.

> - in dvddemux, in the discont event function, you seem to actually do
> specific handling for the current video/audio pad. I'm not sure about
> this, but my personal opinion is that you shouldn't do that. Always handle
> all pads, that's GStreamer. I want to be able to use all audio streams and
> all whatever streams if I have to. That's why current_pad should be a
> ghost pad, not a real pad. I skimmed quickly and couldn't find other
> places where you do this, but if there is any: please don't. I want all
> streams available. MPEG is specifically made for this type of stream
> handling (multi-video, multi-audio, ..).

As I said, single stream pads are available from both mpegdemux and
dvddemux. That was a requirement from you (I guess) and I respected
that. You can use dvddemux for transcoding if you want. Just ignore the
current_* pads and off you go.

> - Style issue: I don't like your CLASS(), MPEG_CLASS() etc. macros. Please
> just use the full names (GST_MPEG_PARSE_GET_CLASS(), which can be called
> on a GstDvdDemux without casting, too).

Ok, I just learned that trick from some other element, don't remember
which one now :-) I can replace them, no problem.

> > - Additionally, the dvddemux element has a notion of current video,
> > audio and subpicture streams, that can be selected trough events (I'll
> > also add properties for that). current_video, current_audio, and
> > current_subpicture source pads give access to the current streams,
> > which, of course, are also available through the standard dynamic pads.
> 
> How do you want to do that? :). Properties? Interface? ...? Integration in
> pads (I'm in favour of this one! matroska, normal mpeg et all have the
> exact same issue...).

Well, that would be as simple as having integer properties
current-video, current-audio and so on, that tell you which stream
number is currently selected (the variables are already there, is just a
matter of exporting them as properties). I'm right now setting them
using DVD events sent from dvdnav, but having a property would allow the
user to override the settings made by the DVD VM (i.e. you are forced to
have subtitles, you turn them off anyway ;-).

M. S.
-- 
Martin Soto <soto at informatik.uni-kl.de>
Universität Kaiserslautern