After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 747774 - dashdemux: text streams support
dashdemux: text streams support
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal enhancement
: 1.7.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 747525
Blocks: 752818
 
 
Reported: 2015-04-13 12:35 UTC by Thiago Sousa Santos
Modified: 2015-12-23 21:12 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Create pads for subtitle streams. (4.59 KB, patch)
2015-07-22 11:01 UTC, Chris Bass
none Details | Review
Create pads for subtitle streams. (8.49 KB, patch)
2015-07-27 15:03 UTC, Chris Bass
none Details | Review
Create pads for subtitle streams. (8.84 KB, patch)
2015-08-18 13:42 UTC, Chris Bass
committed Details | Review

Description Thiago Sousa Santos 2015-04-13 12:35:29 UTC
+++ This bug was initially created as a clone of Bug #747525 +++

There is a playback error when trying to play following dash stream.

- http://dash.edgesuite.net/dash264/TestCases/4b/sony/SNE_DASH_CASE4B_SD_REVISED.mpd

Clone of #737525 to track proper implementation of subtitles support in dashdemux
Comment 1 Chris Bass 2015-07-22 11:01:28 UTC
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.
Comment 2 Thiago Sousa Santos 2015-07-24 12:31:06 UTC
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?
Comment 3 Chris Bass 2015-07-27 13:59:30 UTC
> 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.
Comment 4 Thiago Sousa Santos 2015-07-27 14:09:38 UTC
option B looks better IMHO
Comment 5 Chris Bass 2015-07-27 15:03:28 UTC
Created attachment 308221 [details] [review]
Create pads for subtitle streams.

OK - thanks. Attached patch is modified as suggested.
Comment 6 Chris Bass 2015-08-17 10:21:46 UTC
Ping...
Comment 7 Thiago Sousa Santos 2015-08-17 10:53:43 UTC
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?
Comment 8 Chris Bass 2015-08-17 12:56:20 UTC
(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.
Comment 9 Sebastian Dröge (slomo) 2015-08-18 09:24:48 UTC
(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.
Comment 10 Sebastian Dröge (slomo) 2015-08-18 09:28:32 UTC
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?
Comment 11 Sebastian Dröge (slomo) 2015-08-18 09:43:03 UTC
(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? :)
Comment 12 Chris Bass 2015-08-18 13:42:39 UTC
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.
Comment 13 Sebastian Dröge (slomo) 2015-08-19 07:34:57 UTC
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.
Comment 14 Sebastian Dröge (slomo) 2015-09-25 22:51:26 UTC
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
Comment 15 Sebastian Dröge (slomo) 2015-10-02 10:28:37 UTC
Chris, you mentioned that you have a rendering element for SMPTE TT? Are you planning to propose that for upstream integration?
Comment 16 Chris Bass 2015-10-02 10:50:33 UTC
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.
Comment 17 Sebastian Dröge (slomo) 2015-10-02 10:54:47 UTC
Sounds great, let's discuss that once it's ready then :)