(In reply to comment #0)

>  - the entry types are way too limited, we lose
>     all semantics here. An application will want to
>     know if an entry is a title (dvd), track, chapter,
>     or angle. And if only to present it properly in
>     the user interface (see below)

The idea of the entry types was to have one for "alternatives" (angles, a set of chapters with an alternate ending, ...) and "sequences" (chapters, titles, ...). Maybe there could be a tag or something else for the adding this kind of information to the entries?

>  - how do I represent a DVD with titles that
>    are sub-divided into chapters? I would have
>    to use chapters for the titles, and then
>    chapters for the actual chapters. That
>    doesn't make sense.

titles[chapters]->chapters[chapters]

>  - how do I represent a simple Audo CD with
>    a number of CD tracks? Using 'chapters'
>    here feels a bit wrong

chapters :)

>  - I there's no reason not to, I will add
>     additional entry types for
>     TRACK, TITLE, ANGLE

Maybe make the entry type an entry type "class" (alternative or sequence) and additionally something else to say that this is a track, title, angle, ...?

>  - is an entry's start/stop supposed to
>    be in TIME format? If yes, the function
>    should have _time in it to make that
>    clear IMHO, or one should add a
>    GstFormat parameter (though I think
>    it makes most sense to just stick to
>    time)

TIME, yes

>  - if TIME, relative to what? just whatever
>    makes sense? For a CD track, would it
>    be time relative to the start of the first
>    track, say?

Yes, needs some clarifications probably.

>  - how is an application supposed to
>    present an entry in a UI? Just hope
>    there's a TITLE tag in the taglist ?
>    Should every app create their own
>    _("Chapter/Title/Track/Angle %d") ?
>    Note that titles may not be numbered
>    in sequence, e.g. one might only
>    expose Title 1 and Title 10 on a DVD.

My idea was to use the TITLE tag, yes. For some formats there are even titles for the chapters for example

>  - the whole 'info' structure thing is
>    weird. The fact that there is such an
>    'info' structure should be hidden as
>    an implementation detail behind API
>    that allows elements to add an
>    additional GstStructure of their
>    own (similar to qdata). If it then
>    stores that inside another 'info'
>    GstStructure or a GList or whatever
>    should be an implementation detail.
>    There should be setter/getter API
>    that takes either a string or
>    a quark or a GstObject as key and
>    then a GstStructure as value.

Agreed

>  - (there should generally be
>     getter/setter API, but that's just
>     a nitpick compared to the other
>     issues)

What do you mean with getter/setter API?

>  - what's with the recommendation/
>    tendency to start every TOC with
>    a top-level edition entry, even if
>    there's only one actual TOC ?
>    I find this weird semantically,
>    and it makes things much harder
>    for apps to handle, and to decide
>    what to use and what to expose.

If there's only one edition and you don't have specific metadata for it, just start with the chapters at the top-level?
What would you suggest instead?

>    If there can be multiple sources
>    for a TOC, there should be
>    multiple GstTocs IMHO. A different
>    "edition" of the toc itself is not
>    the same thing as a different
>    edition of the medium. But
>    perhaps there's a misunderstanding
>    on my part here. If not, then I
>    would suggest we add some kind
>    of gst_toc_add_alternate() or so.
>    That would get rid of the top
>    level edition thing, and also makes
>    it clear to apps which TOC they
>    should use/prefer etc. (i.e. they
>    can just ignore the alternates).

For that maybe we should add a "primary" or "priority" field to the entries. For the meaning of edition and chapter see above.

> - (some more guidance on the
>    uid thing would be nice in the
>    docs, what it is supposed to
>    look like ideally for example)

Whatever you like to have in there, just some unique string representating the entry. For some formats you could use the in-stream UIDs for example, for others things like "Title 2/Chapter 17/Angle 2"

Also note that this API is in 0.10 too

> The idea of the entry types was to have one for "alternatives" (angles, a set
> of chapters with an alternate ending, ...) and "sequences" (chapters, titles,
> ...). Maybe there could be a tag or something else for the adding this kind of
> information to the entries?

What do you mean with 'this kind of information' here? The track/title/chapter thing or the sequence thing?

Why not just make it explicit? I understand the original distinction, I'm just saying that it doesn't make sense to drop the distinction between titles/tracks/chapters and that applications would want to know which one it was. It's also less intuitive.


> titles[chapters]->chapters[chapters]

It doesn't make sense, sorry. At the very least the CHAPTERS enum is completely misnamed and should be ENTRY_TYPE_SEQUENCE_THINGY then. Which perhaps makes it more obvious how absurd this is.


> >  - how do I represent a simple Audo CD with
> >    a number of CD tracks? Using 'chapters'
> >    here feels a bit wrong
> 
> chapters :)

It's just wrong.

 
> >  - I there's no reason not to, I will add
> >     additional entry types for
> >     TRACK, TITLE, ANGLE
> 
> Maybe make the entry type an entry type "class" (alternative or sequence) and
> additionally something else to say that this is a track, title, angle, ...?

Could do two different enums, or something with flags, or add API to check whether it's a sequence or alternative.

 > >  - (there should generally be
> >     getter/setter API, but that's just
> >     a nitpick compared to the other
> >     issues)
> 
> What do you mean with getter/setter API?

The structs should be opaque instead of people poking at the members.

 
> If there's only one edition and you don't have specific metadata for it, just
> start with the chapters at the top-level?
> What would you suggest instead?

That seems fine.


> Also note that this API is in 0.10 too

I know, but I don't care about that and I don't intend to spend time fixing it up in 0.10.

(In reply to comment #0)
> Some quick comments on the GstToc API after attempting to add TOC support to
> GstAudioCdSrc.
> 
>  - toc and toc entry should be refcounted. They
>     are usually created once and then only read,
>     but not modified (done: made them mini objects)

Cool :)

>  - the entry types are way too limited, we lose
>     all semantics here. An application will want to
>     know if an entry is a title (dvd), track, chapter,
>     or angle. And if only to present it properly in
>     the user interface (see below)

Well, at the begining the main idea was to make it as much general and abstract as possible to support most of media formats' needs. Yes, maybe naming 'chapter' isn't quite good enough. I like an idea to enhance toc entry. 

>  - how do I represent a DVD with titles that
>    are sub-divided into chapters? I would have
>    to use chapters for the titles, and then
>    chapters for the actual chapters. That
>    doesn't make sense.

We can add more entry types (CHAPTER, TRACK, TITLE, ANGLE, etc) and rename existing to SEQUENCE and ALTERNATIVE. The latter two are more abtract. Then it would be: [TITLE]->sub-chapters. I think that two enums for entry typing will introduce more problems.

>  - how do I represent a simple Audo CD with
>    a number of CD tracks? Using 'chapters'
>    here feels a bit wrong

[SEQUENCE]->tracks

>  - I there's no reason not to, I will add
>     additional entry types for
>     TRACK, TITLE, ANGLE

Sure.

>  - is an entry's start/stop supposed to
>    be in TIME format? If yes, the function
>    should have _time in it to make that
>    clear IMHO, or one should add a
>    GstFormat parameter (though I think
>    it makes most sense to just stick to
>    time)
>
>  - if TIME, relative to what? just whatever
>    makes sense? For a CD track, would it
>    be time relative to the start of the first
>    track, say?

They are atored in nanoseconds (gint64) from the start of the file/stream. It's my fault that it is not cleared in documentation.
 
>  - how is an application supposed to
>    present an entry in a UI? Just hope
>    there's a TITLE tag in the taglist ?
>    Should every app create their own
>    _("Chapter/Title/Track/Angle %d") ?
>    Note that titles may not be numbered
>    in sequence, e.g. one might only
>    expose Title 1 and Title 10 on a DVD.

Yeap, application should look for TITLE tag. So the element implementing GstToc
must name every entry. To simplify developers life we can add setter/getter for the title, which will wrap TITLE tag inside. Well, there are songs without artist and title tags. And it's OK. Any ideas how to handle entries with empty titles? Maybe application should care about it or we can make helper function which will give names to empty titles in order of TOC processing? Or application can use entry's UID instead of empty title.

>  - the whole 'info' structure thing is
>    weird. The fact that there is such an
>    'info' structure should be hidden as
>    an implementation detail behind API
>    that allows elements to add an
>    additional GstStructure of their
>    own (similar to qdata). If it then
>    stores that inside another 'info'
>    GstStructure or a GList or whatever
>    should be an implementation detail.
>    There should be setter/getter API
>    that takes either a string or
>    a quark or a GstObject as key and
>    then a GstStructure as value.

Agree. I think having two internal GstStructures (one for internal info and another for user specific structures/data) would be nice. And add an getter/setter API with quark and GstStructure. Later we can use this userspace structure to store additional user specific data.


>  - what's with the recommendation/
>    tendency to start every TOC with
>    a top-level edition entry, even if
>    there's only one actual TOC ?
>    I find this weird semantically,
>    and it makes things much harder
>    for apps to handle, and to decide
>    what to use and what to expose.

This design was taken from matroska format. But I don't see any limitations not to store entries right inside the TOC.
 
>    If there can be multiple sources
>    for a TOC, there should be
>    multiple GstTocs IMHO. A different
>    "edition" of the toc itself is not
>    the same thing as a different
>    edition of the medium. But
>    perhaps there's a misunderstanding
>    on my part here. If not, then I
>    would suggest we add some kind
>    of gst_toc_add_alternate() or so.
>    That would get rid of the top
>    level edition thing, and also makes
>    it clear to apps which TOC they
>    should use/prefer etc. (i.e. they
>    can just ignore the alternates).

I think combining several GstTocs together isn't a good idea. Resulting structure can be really huge. I like an idea to add "priority" field so the application can decide which one to use.

> - (some more guidance on the
>    uid thing would be nice in the
>    docs, what it is supposed to
>    look like ideally for example)

Agree. A bit of explaining is in the GstTocEntry description.

Uhf, a head is boiling a bit, maybe clearer thoughts will be at the morning :)
If we agree on implementation details I can do this work on GstToc.

> We can add more entry types (CHAPTER, TRACK, TITLE, ANGLE, etc) and rename
> existing to SEQUENCE and ALTERNATIVE. The latter two are more abtract. Then it
> would be: [TITLE]->sub-chapters. I think that two enums for entry typing will
> introduce more problems.

Ooops o_O
Of course we need to indicate whether entry is of 'edition' type or not. Maybe just an setter/getter API would be enough (gst_toc_entry_is_alternative() and get_toc_entry_set_alternative())? And entry type would be CHAPTER, TRACK, TITLE, ANGLE, EDITION, etc itself.

Created attachment 217249 [details] [review]
toc: add more entry types

Another thing I was wondering is how the toc event is supposed to be used or where it is useful.

I saw that in matroskamux the toc event is basically handled like taglists and simply used as an alternative TOC if one hasn't been set by GstTocSetter. I'm not sure that's right.

For example: Let's say I have a pipeline that encodes an audio track, like this:

   gst-launch-1.0 cdparanoiasrc track=1 ! audioconvert ! encoder ! matroskamux ! filesink

The cdparanoiasrc will create a TOC on start, and post a TOC message on the bus (so far so great). Now the question is: should it also push a TOC event downstream? I don't think matroskamux should create a TOC from that info, since it's just one single track that's being muxed here.

On the other hand, imagine

   gst-launch-1.0 dvdreadsrc title=1 ! encoder ! matroskamux ! filesink

It would totally make sense for matroskamux to write the chapter information as TOC in the file. The problem is just that the TOC contains all the info for the other titles too.

So I think what we need is either

a) a way for the muxer to tell what position/entry the current stream represents in the TOC hierarchy (top-level, or title/track/uid N)

b) for the source to generate a different stream/title-specific TOC and send that as event downstream (so dvdreadsrc would send a TOC with the chapter info, but cdparanoiasrc would not send a TOC at all).

In general, I wonder how the whole TocSetter/muxer thing is supposed to work... - is it right that a matroska file should only contain a TOC if it contains multiple tracks, say? (or multiple chapters for a movie)

(In reply to comment #7)
> [...]
> a) a way for the muxer to tell what position/entry the current stream
> represents in the TOC hierarchy (top-level, or title/track/uid N)
> 
> b) for the source to generate a different stream/title-specific TOC and send
> that as event downstream (so dvdreadsrc would send a TOC with the chapter info,
> but cdparanoiasrc would not send a TOC at all).

I think b) is the nicer solution.

> In general, I wonder how the whole TocSetter/muxer thing is supposed to work...
> - is it right that a matroska file should only contain a TOC if it contains
> multiple tracks, say? (or multiple chapters for a movie)

What do you mean?

> I think b) is the nicer solution.

Yes, agreed.

 
> > In general, I wonder how the whole TocSetter/muxer thing is supposed to work...
> > - is it right that a matroska file should only contain a TOC if it contains
> > multiple tracks, say? (or multiple chapters for a movie)
> 
> What do you mean?

Well, we don't really have the situation covered how to create a file that contains multiple titles/tracks in general (e.g. stuff all audio tracks of a CD into a matroska file, or multiple related video files). I don't think we need to solve that now though :)

I think we've resolved the open conceptual issues now, so let's close this.

Still need to implement sending TOCs with current scope where appropriate though (e.g. audiocdsrc in continuous mode).

commit 05daa261f15e8f65a60adc4f5adc01b556e1817c
Author: Tim-Philipp Müller <tim@centricular.net>
Date:   Sat Jul 28 11:02:30 2012 +0100

    docs: update TOC design docs a little

commit 85456357dd26f15ad1cc17e93c2aec818e33a6db
Author: Tim-Philipp Müller <tim@centricular.net>
Date:   Sat Jul 28 09:41:30 2012 +0100

    event: make TOC event multi-sticky
    
    We need to send two kinds of TOCs downstream as events,
    and need both to stick to the pads.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=678742

commit ee6ab7c93638a6519acb976699a6ad149d520a95
Author: Tim-Philipp Müller <tim.muller@collabora.co.uk>
Date:   Sat Jul 28 08:30:36 2012 +0100

    tools: print TOC scope

commit e8ab1006c701ee5dbcbcd90c75800f86e3c28ba8
Author: Tim-Philipp Müller <tim.muller@collabora.co.uk>
Date:   Fri Jul 27 23:56:54 2012 +0100

    toc: add GstTocScope and require it in the constructor
    
    This is because we need to be able to signal different TOCs
    to downstream elements such as muxers and the application,
    and because we need to send both types as events (because
    the sink should post the TOC messages for the app in the
    end, just like tag messages are now posted by the sinks),
    and hence need to make TOC events multi-sticky.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=678742