-
Notifications
You must be signed in to change notification settings - Fork 6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add DVB subtitles support #1781
Conversation
@ojw28 Could you say if you approve our method to add support for graphic subtitles? our next patch (teletext subtitles) need support of graphic chars and we want to build it´s support in top of this pull request. |
9b72586
to
b97e9c8
Compare
e69d8f3
to
ce2a80f
Compare
Any update on this? I'd love to see it wrapped up and merged, as it would be very useful for my app (See kiall/android-tvheadend#36) where we receive mostly raw satellite feeds from a local Satellite to IP server. Transcoding content is possible (albeit, at the cost of server CPU time), but tvheadend doesn't offer subtitle transcoding, so we're out of luck there. |
@kiall - We asked for clarification about the CLA with respect to this pull request in #179 and didn't receive any. We wont be looking in detail until clarification is provided. Note that a piece of the functionality needed to support bitmap subtitles, which overlaps will some of this pull request, is likely to be merged via #2219. |
I was not aware of any pending CLA clarifications, just checked the email. In any case from the last conversation with @AquilesCanta and you, I assumed that you were not interested in support this code directly on ExoPlayer and that you will provide a way to do this out of tree, since then we are porting our DVB subtitle patches to use #726, but even if we finish this port (new features keeps us from finishing this) we need graphic subs support in the subtitle subsystem 530977d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more generic, and perhaps attractive api change for exoplayer could be to generalize subtitles into overlays. There could be text-based overlays and graphics-based overlays. Subtitle then, would be a type of overlay. Ads, news or live-discussions could be other types of overlays.
This patch places some Bitmap code in the exoplayer2.text package, which they may not like.
Something along these lines was our first approach but then we decide to maintain the changes on the ExoPlayer library at a minimum to have better chances to get the patches accepted. But for our VR player we are considering passing two surfaces to ExoPlayer to separate video and overlays and have more compositing possibilities (head aligned overlays, ...) |
The plumbing of bitmaps through the text package is likely to be merged in via #2219. We've no objection to that change, and it's more or less required to enable support for bitmap subtitles to be added via extension in a clean way. Whether we then support DVB directly so that you don't need to do any extension is being re-assessed as per #179. It's more likely than it was in the past, at least. The CLA question isn't about whether the CLA is signed. It's about whether the code is an original creation as per the requirements of the CLA. One of the changes in this PR has "Original C code taken from VLC project" written in it. I don't think you can take GPL licensed code, make a derivative work and then remove the license. In general we can only accept original code where the contributor owns the copyright/IP and where it's not subject to other licenses. If you believe the PR does meet the requirements of the CLA, please clarify. Thanks! |
Re the more general overlay suggestion: As soon as you're thinking about arbitrary overlays (e.g. live discussions) you probably also want things like custom animations, fades, clever interactions with playback controls etc. I don't think it's possible to come up with a general way of modeling that, short of reimplementing the whole Android UI layer. So I think the best way to support such cases is just to provide a way for applications to insert custom layouts on top of the video where they can do whatever they like. Which is something you can already do. There are a whole bunch of ways to do this, for example overriding exo_simple_player_view.xml (see this post for details). |
I suppose that a port of a LGPL code is a derivative work, so in his
current state the including library will have to comply with the LGPL. We
don't want to reimplement the lib from scratch so we will create a separate
library with LGPL code that links with ExoPlayer and use a custom
TsPayloadReader to inject de DVB subs reader, this way we will be able to
redistribute the library with the appropriate license attached and without
ExoPlayer code being tainted by LGPL code.
…On Thu, Dec 29, 2016 at 3:04 PM, ojw28 ***@***.***> wrote:
The plumbing of bitmaps through the text package is likely to be merged in
via #2219 <#2219>. We've no
objection to that change, and it's more or less required to enable support
for bitmap subtitles to be added via extension in a clean way. Whether we
then support DVB directly so that you don't need to do any extension is
being re-assessed as per #179
<#179>; it's more likely than
it was in the past, at least.
The CLA question isn't about whether the CLA is signed. It's about whether
the code is an original creation as per the requirements of the CLA. One of
the changes in this PR has "Original C code taken from VLC project" written
in it. I don't think you can take GPL licensed code, make a derivative work
and then remove the license. In general we can only accept original code
where the contributor owns the copyright/IP and where it's not subject to
other licenses. If you believe the PR does meet the requirements of the
CLA, please clarify. Thanks!
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1781 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AJ99vwpL61AKCLAJOoh6P7spwBGoyI3jks5rM733gaJpZM4Jt1YP>
.
|
@tresvecesseis - We've now merged the rendering changes required to support DVB subtitles (i.e. the required changes to
This will result in the track being visible, but ignored as not supported by default. Enabling support in your app will not require any changes within the library itself. You'll be able to simply pass your own Note: This is conditional on the TsExtractor changes being an original work (as far as I can see the LGPL derivative work is only in the decoder, but please confirm if you're interested in doing this). Note2: We would also accept a pull request to add a Thanks! |
@ojw28 – We were prioritizing other features in our v2 porting efforts, but we will review the necessary changes and come back to you with an ETA.
From: ojw28 <notifications@github.com>
Reply-To: google/ExoPlayer <reply@reply.github.com>
Date: Monday, 27 February 2017 at 16:34
To: google/ExoPlayer <ExoPlayer@noreply.github.com>
Cc: "tresvecesseis@gmail.com" <tresvecesseis@gmail.com>, Mention <mention@noreply.github.com>
Subject: Re: [google/ExoPlayer] Add DVB subtitles support (#1781)
@tresvecesseis - We've now merged the rendering changes required to support DVB subtitles (i.e. the required changes to Cue and SubtitleView). It would be cool if we could get to the point where adding DVB support is as simple as inserting the additional subtitle decoder when building an ExoPlayer instance. Specifically, this means I think it would be fine for you to propose a pull request that merges:
The addition of the DVB mime type to MimeTypes.java
Any remaining change necessary to Format.java for your use case
Changes to TsExtractor that expose the track
This will result in the track being visible, but ignored as not supported by default. Enabling support in your app will not require any changes within the library itself. You'll be able to simply pass your own SubtitleDecoderFactory that's capable of instantiating a DvbSubtitleDecoder, which will live in your application code (or some other library, as appropriate).
Note: This is conditional on the TsExtractor changes being an original work (as far as I can see the LGPL derivative work is only in the decoder, but please confirm if you're interested in doing this).
Thanks!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
I have been studying the needed changes to the ExoPlayer library and we have found that we only need to add the DVB mime type to MimeTypes.java, the other changes could be done in the player with our own TsPayloadReaderFactory and SubtitleDecoderFactory. We will finish our port and come back to you if we find anything else. |
@ojw28, It´s possible to make the Ts readers classes public to be able to create a custom TsPayloadReaderFactory? |
We will be pushing the visibility change soon. |
@tresvecesseis - You may still want to consider sending a pull request to contribute the TsExtractor changes, despite the fact you're able to inject them via |
@ojw28, I was worried about the posible memory leak if I wire up a reader but there is not a subtitle decoder able to consume these buffers. |
@ojw28 , I've implemented all your requests, tomorrow I will send some samples of dvb subtitles in matroska and mp2t containers |
I've re-tested with the latest changes, and have found no issues with the various DVB-S streams I've tested with (other than the colored debug border around the subtitles). Re samples - here are two sample MKV files, ffprobe output below: https://www.dropbox.com/s/mb9r314tc2m6taj/The%20Big%20Bang%20Theory.mkv?dl=1 These are both short recordings of programs broadcast over DVB-S which each contain a DVBSUB and a subrip subtitle track.
|
Thanks. We're working on getting this change merged. |
@kiall - Any idea why the audio tracks in those samples have the codec ID set to |
@ojw28 Humm - ffprobe shows these are mp2 audio, VLC also shows these as "MPEG Audio L1/2". mkvinfo however does show Looking at the muxer code, they have a bug: https://github.com/tvheadend/tvheadend/blob/master/src/muxer/muxer_mkv.c#L293-L296 |
pixel-data_sub-block() | ||
while (processed_length<bottom_field_data_block_length) | ||
pixel-data_sub-block() | ||
if (!wordaligned()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it's not implemented (and needs to be). I assume you're supposed to skip 0, 1, 2 or 3 bytes so that you're a multiple of 4 from the start of object_data_segment.
Don't worry about updating the change; I'm working on some changes to merge with it and will put a fix in as part of that. But let me know if the above sounds right. It's not actually defined in the spec what the word length is, as far as I can tell!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably more robust just to use the section length and skip to the end of the section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is taken into account in code beginning at line 1410:
+ // test for segment Sync Byte and account for possible additional wordalign byte in Object data segment
+ while (sync == 0x0f || (sync == 0x00 && (sync = tsStream.readBits(8)) == 0x0f)) {
+ parseSubtitlingSegment();
+ if (isSet(FLAG_PES_STRIPPED_DVBSUB) && tsStream.bitsLeft() == 0) {
+ break;
+ }
+ sync = tsStream.readBits(8);
+
+ }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was done that way because we also didn't know the word length
re MP2/MP3 issue - I'll raise this with the TVHeadend folks, in the meantime, I've corrected both samples. Please re-download. |
@ojw28 , I have shared with you a google drive folder with some test streams, it even contains a stream with two subs in one PID that, with the current patch, have two problems; only the first subtitle is shown and the two tracks are in french language but one is for the hearing impaired and we have removed the information about the subtitlingType because there is no way to signal this feature |
@ojw28 , there are more samples in: http://samples.mplayerhq.hu/sub/dvbsub/ but the french one is the only ts that is playable in the demo player, I didn't investigate the problem but other players have no issues with them |
@ojw28 , finally I decided to implement the multiple subtitles track per PID the ugly way, at least we are feature complete now, fell free to drop the last patch if you think there is a better way |
05d25a2
to
e6f2b88
Compare
…ng impaired subtitles
e6f2b88
to
38779de
Compare
" (x/y): (" + object.objectHorizontalPosition + "/" + object.objectVerticalPosition + ")"); | ||
} | ||
|
||
region.regionObjects.put(arrayIndex++, object); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If L783 is executed, this line also affects tempRegion.regionObjects. Is that intentional, or should L783 make a copy of the SparseArray rather than just taking a reference to it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why the I added the additional variable, to keep adding elements to the array in case of a page composition update, see the last part of the BBC stream sample, where words are added incrementally to a region
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My question is whether it's intentional that both the local object
and subtitleService.regions.get(region.regionId)
are being modified on this line (because they're both referencing the exact same regionObjects
array). Intuitively I'd expect L783 to copy the elements from tempRegion.regionObjects
into region.regionObjects
, not just update the reference. That would result in only object
being modified on this line.
Oh, right. Looking deeper it looks like subtitleService.regions
is updated to remove tempRegion
anyway after the call to this method complete at L523, so it doesn't make much difference.
region.regionDepth = tsStream.readBits(3); | ||
tsStream.skipBits(2); | ||
region.clutId = tsStream.readBits(8); | ||
tsStream.skipBits(16); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason region8BitPixelCode/region4BitPixelCode/region2BitPixelCode aren't parsed and set on the region here? They're currently not set anywhere in code, but they are read (and will always have a value of 0).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nop, I forgot to set the values when I implemented the fill region part.
" (w/h): (" + region.regionWidth + "/" + region.regionHeight + ")"); | ||
} | ||
|
||
int arrayIndex = 0; // index by an incremental counter to allow repeating objects in one region |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of this variable? It looks like it's always equal to the number of items in the regionObjects SparseArray. I think it's equivalent to get rid of this variable and just use an ArrayList. It looks like it's always true that b == objectKey
at L1496 as well.
German DVB-S2 brodcast sample which contains a DVBSUB:
|
Merged. Thanks a lot for your help on this! I ended up doing quite a bit of cleanup, but it was all pretty mechanical. Mainly making things static and final where-ever possible. For parser code in particular, if you can get to the point where you have:
where Note: The last commit on this pull request got clobbered as part of the cleanup. Sorry about that. Please feel free to send a new pull request that makes the equivalent change on top of the cleaned up code! |
When testing the newly merged dev-v2 version, I noticed that when there are dvbsubs and cea-608 tracks detected in the stream, it will not select/activate dvbsubs by default:
But when opening another sample with x-subrip subtitles, the first track is selected and activated by default:
Is this expected behaviour? |
If the media indicates that a subtitle should be enabled by default then we enable it. Else we don't. So yes, this is expected behavior where the first sample doesn't mark a subtitle track as default and the second sample does. For MKV I think tracks are marked using FlagDefault (see the spec here). |
Here is a TS sample with DVB Teletext subtitles that are currently not detected by ExoPlayer: https://www.dropbox.com/s/smqq070j0cxug90/NAR_subtitles.ts?dl=0
|
@needz that sample has a DVB teletext stream, not a DVB subtitles stream (the ffprobe output naming is confusing, but that's what it says) |
@kiall, yes, but in this case the subtitles are brought using Teletext (some broadcasters use this instead of DVB subtitles) and most of the devices/players detect and show them just fine. I thought it could be a next step to implement in ExoPlayer in the future. |
Added support for graphic subtitles and use that infrastructure to provide DVB subtitles over MP2T