Created attachment 307900 [details] [review]
Create pads for subtitle streams.

In the DVB DASH profile, Representations that carry timed text subtitles (or the AdaptationSet containing those Representations) carry the attribute codecs="stpp". The code in the patch checks whether application/mp4 streams have this codec value set. If so, it creates a subtitle src pad; if not, no pad is created for the stream.

The "stpp" codec type is defined at http://www.mp4ra.org/codecs.html to indicate timed text subtitles rather than just generic timed text; therefore, I've called the created src pads "subtitle_X" rather than "text_X".

As regards the caps of the pads, what's needed is a new caps type that represents "timed text encapsulated in ISO BMFF". I've struggled to think of the best name for this; in the end I've gone with application/x-mp4-text.

Review of attachment 307900 [details] [review]:

::: ext/dash/gstdashdemux.c
@@ +866,3 @@
+    caps = gst_caps_from_string ("application/x-mp4-text");
+  else
+    caps = gst_caps_from_string (mimeType);

How about moving this stream_contains_subtitles to mpdparser as the other mimetype to caps conversions are being done there. The difference here is that you need also to check the codec field. What do you think of adding a new gst_mpd_client_get_stream_caps() that would handle the previous and this new case?

> How about moving this stream_contains_subtitles to mpdparser as the other
> mimetype to caps conversions are being done there. The difference here is
> that you need also to check the codec field. What do you think of adding a
> new gst_mpd_client_get_stream_caps() that would handle the previous and this
> new case?

Sure. Do you think it's best to (a) add a new function gst_mpd_client_get_stream_caps(), or (b) to rename gst_mpd_client_get_stream_mimeType() to gst_mpd_client_get_stream_caps(), extending it to handle the subtitle cases and having it return a GstCaps rather than gchar*?

At the moment, gst_mpd_client_get_stream_mimeType() isn't really returning the mimeType of the streams - it calls gst_mpdparser_mimetype_to_caps to convert the mimeTypes to caps strings, so its name is perhaps a little bit misleading at the moment.

option B looks better IMHO

Created attachment 308221 [details] [review]
Create pads for subtitle streams.

OK - thanks. Attached patch is modified as suggested.

Ping...

Patch looks good, we just need to settle on the caps that we are going to use. Is there an official mimetype for these kind of files or are they just considered mp4 just like regular files?

Any reason for using "application/x-mp4-text" other than being able to know from caps that it is a text-only media?

(In reply to Thiago Sousa Santos from comment #7)
> Patch looks good, we just need to settle on the caps that we are going to
> use. Is there an official mimetype for these kind of files or are they just
> considered mp4 just like regular files?

I can't find an official mimetype for text in mp4; there's just application/mp4 for mp4 that doesn't contain either audio or video.

> Any reason for using "application/x-mp4-text" other than being able to know
> from caps that it is a text-only media?

It was only really to have a type that specifically identified media as text encapsulated in mp4. I guess it might be possible to be less specific and use application/mp4 instead - a quick grep through the base/good/bad source code shows that it's not being used in elements, so hopefully shouldn't break any autoplugging.

Another option would be to suffix some more specific information onto the end of application/mp4, for example application/mp4+stpp or application/mp4+text.

(In reply to Chris Bass from comment #8)
> (In reply to Thiago Sousa Santos from comment #7)
> > Patch looks good, we just need to settle on the caps that we are going to
> > use. Is there an official mimetype for these kind of files or are they just
> > considered mp4 just like regular files?
> 
> I can't find an official mimetype for text in mp4; there's just
> application/mp4 for mp4 that doesn't contain either audio or video.

As this is a completely normal MP4 container with a standard ftyp, I think just using video/quicktime is ok. There's nothing special about it that would require a distinction, right?

Just reuse the existing media format name.

Review of attachment 308221 [details] [review]:

Looks generally good. But this wouldn't handle timed text without ISOBMFF encapsulation too?

::: ext/dash/gstdashdemux.c
@@ +732,3 @@
       break;
+    case GST_STREAM_APPLICATION:
+      if (gst_mpd_client_active_stream_contains_subtitles (stream)) {

Can there be a stream that contains subtitles *and* audio/video media?

(In reply to Sebastian Dröge (slomo) from comment #10)
> 
> But this wouldn't handle timed text without ISOBMFF encapsulation too?

If I understand this correctly, for the one without encapsulation the mimetype would just be application/ttml+xml instead of application/mp4. So should be easy to add. Let's just do that together with this patch? :)

Created attachment 309473 [details] [review]
Create pads for subtitle streams.

Updated patch. Now sets video/quicktime as caps for timed-text encapsulated in ISO BMFF and also handles unencapsulated timed-text subtitle Representations.

Review of attachment 309473 [details] [review]:

Looks good to me

::: ext/dash/gstmpdparser.c
@@ +4426,3 @@
   }
 
+  caps_string = gst_mpdparser_mimetype_to_caps (mimeType);

Maybe we should be more restrictive in that function, and only whitelist some mimetypes instead of converting every unknown mimetype directly to caps? But if so, in a follow-up patch.

commit 69f86e51b2f64608a500b98af0edda0e6d4e95c2
Author: Chris Bass <floobleflam@gmail.com>
Date:   Tue Aug 18 14:16:11 2015 +0100

    dashdemux: create src pads for subtitle streams.
    
    Create src pads for Representations that contain timed-text subtitles,
    both when the subtitles are encapsulated in ISO BMFF (i.e., the
    Representation has mimeType "application/mp4") and when they are
    unencapsulated (i.e., the Representation has mimeType
    "application/ttml+xml").
    
    https://bugzilla.gnome.org/show_bug.cgi?id=747774

Chris, you mentioned that you have a rendering element for SMPTE TT? Are you planning to propose that for upstream integration?

Yes, that's the plan (it's an EBU-TT-D rendering element, rather than SMPTE TT).

It's pretty much ready; I'm just going through the process of getting permission from my employer to release it.

How to integrate it will require some discussion. At the moment it's split into two elements - a parser and a renderer - with a library defining types used to exchange data between the parser and renderer.

The parser element is an extension to subparse. The changes are fairly self-contained, consisting of a new file containing functions that handle EBU-TT-D, and some minimal changes to gstsubparse.c

The renderer element is a modified version of basetextoverlay; so how best to integrate that might be a bit more complicated.

Anyway, my plan is to put the code up on github first, and then we can work out what to do from there.

Sounds great, let's discuss that once it's ready then :)