Add minimal support of DVB AIT #6922

phhusson · 2020-01-28T15:13:49Z

This is used by broadcast channels to provide interactive contents, through HbbTV.

There are many more informations available in this TS section, though this is enough for a minimal HbbTV implementation.

This is used by broadcast channels to provide interactive contents.

googlebot · 2020-01-28T15:13:52Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

phhusson · 2020-01-28T15:15:16Z

@googlebot I consent.

googlebot · 2020-01-28T15:15:20Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

phhusson · 2020-01-28T15:18:13Z

Test stream: https://treble.phh.me/test-ait.ts (this link won't live very long, don't use the link for permanent tests)

ojw28 · 2020-01-30T19:42:01Z

@icbaker - Could you take a look at this, since it falls under metadata?

icbaker

Could you also please look at adding some tests for AitDecoder?
There's some other metadata decoder tests in the icy and scte35 packages for inspiration.

If you have a (short & small!) TS stream with an AIT that can be included in the PR you can also add it to TsExtractorTest which will add some nice robustness. This might be a bit awkward to get working, so feel free to reach out for help if it's not working as you'd expect.

icbaker · 2020-01-31T16:50:52Z

library/core/src/main/java/com/google/android/exoplayer2/metadata/dvbsi/AitDecoder.java

+        int endOfSection = sectionData.getPosition() + (tmp & 4095) - 4 /* Ignore leading CRC */;
+
+        tmp = sectionData.readUnsignedShort();
+        int applicationType = tmp & 0x7fff;


Because a lot of the values don't fall on clean byte boundaries, but they are mostly numeric, i think you might be better off using ParsableBitArray instead of ParsableByteArray

Then you can read and skip bits and it should be super-clear that you've implemented the spec.

It makes reading strings a bit more awkward, but you can use Util.fromUtf8Bytes to help I suspect (or look into adding string reading methods to ParsableBitArray.

Done. I ended up adding readString() to ParsableBitArray

icbaker · 2020-01-31T17:04:12Z

library/core/src/main/java/com/google/android/exoplayer2/metadata/dvbsi/AitDecoder.java

+import java.util.ArrayList;
+
+public class AitDecoder implements MetadataDecoder {
+    // Specification of AIT can be found in 5.3.4 of TS 102 809 v1.1.1


If this is meant to be explaining the DESCRIPTOR_TRANSPORT_PROTOCOL constant, it seems like 5.3.6 is a more precise reference?

I think we should add similar references to the other constants too to point to the relevant sections of the spec

Well no I really meant to link this for global AIT documentation
I updated the documentation structure to make more sense

icbaker · 2020-01-31T17:16:14Z

library/core/src/main/java/com/google/android/exoplayer2/metadata/dvbsi/AitDecoder.java

+                    int label = sectionData.readUnsignedByte();
+
+                    if(protocolId == TRANSPORT_PROTOCOL_HTTP) {
+                        while (sectionData.getPosition() < positionOfNextSection2) {


instead of using a 2 suffix, can we call this positionOfNextSelector (and also rename positionOfNextSection to positionOfNextDescriptor above)

Same also for any of the l or len variables, especially if their scope is quite large, could we qualify them with what they're the length of?

(this is assuming I've correctly matched up your code with the names used in the spec! Please correct me if I'm making nonsense suggestions)

Right, that's indeed a mess, I've refactored this.
FWIW I don't end with the same renaming as you.
Hopefully I added enough comments and reference to the standard so we can converge :-)

icbaker · 2020-01-31T17:18:37Z

library/core/src/main/java/com/google/android/exoplayer2/metadata/dvbsi/AitDecoder.java

+        null,
+    };
+
+    static String readDvbString(ParsableByteArray data, int length) {


Is this method used? I couldn't see a reference to it

Correct, it isn't used.
That was used by some dead code (there are many informations not parsed by this decoder) (namely to get the "Application name")

This will be removed, thanks

icbaker · 2020-01-31T17:19:29Z

.../src/main/java/com/google/android/exoplayer2/extractor/ts/DefaultTsPayloadReaderFactory.java

      default:
        return null;
    }
  }

+  public class SectionPassthrough implements SectionPayloadReader {


Can you add some documentation about what this class is for/what it does?

icbaker · 2020-01-31T17:20:02Z

.../src/main/java/com/google/android/exoplayer2/extractor/ts/DefaultTsPayloadReaderFactory.java

+  public class SectionPassthrough implements SectionPayloadReader {
+    private TimestampAdjuster timestampAdjuster = null;
+    private final String mimeType;
+    private TrackOutput output;


Can this be final?

I can't see how, since output can be created only in init() function not in constructor.
Looking at other Readers (Ac3Reader), I see that trackouput is @MonotonicNonNull which I'll add.

icbaker · 2020-01-31T17:21:04Z

library/extractor/src/main/java/com/google/android/exoplayer2/extractor/ts/TsExtractor.java

@@ -96,6 +96,9 @@
  public static final int TS_STREAM_TYPE_SPLICE_INFO = 0x86;
  public static final int TS_STREAM_TYPE_DVBSUBS = 0x59;

+  //Those are special IDs, which don't have actual TS definitions


Where does the value come from? Can you reference a spec or something?

No, this value is magic value that is beyond max standard value of TS_STREAM_TYPE which is one byte.
It is meant to carry stream type for types not defined in the spec.

Do you have a recommendation to handle that?

Sorry to jump in: You can avoid the public magic value by adding a boolean in StreamInfo saying "hasApplicationSignallingDescriptor" which is true if and only if one of the descrptor uses descriptor tag 0x6F. Alternatively, you could include in stream info a list / array of all descriptor tags associated with the program, and then checking for that in the TsPayloadReaderFactory. The boolean is probably just simpler.

For what it's worth, Teletext tracks are also detected by a specific descriptor in PMT
(see https://www.etsi.org/deliver/etsi_EN/300400_300499/300472/01.03.01_60/en_300472v010301p.pdf section 4)

So the boolean way doesn't work much. I'm not a fan of including all descriptors tags.
We could have an enum ExtraType { Nope, Application, Teletext, } but I'm not much of a fan either.

Now, in Teletext's case, descriptors contain useful information to send to Teletext{Reader,Decoder} (namely the page of the subtitle and the language). At the moment we transmit it by creating a new TelextSubtitleInfo storage-class (which inherits SubtitleInfo, and we changed DvbSubtitleInfo inherits SubtitleInfo as well). And we gave teletext track 0x102 id

I don't have strong opinion on how to handle that, though at the moment I still prefer leaving those "internal" IDs.

…ndard

phhusson · 2020-02-18T14:27:19Z

There I did the few bits of refactoring.
I don't know if you prefer that I squash all my patches, or I fix your remarks commit by commit?

I'll now add tests.

phhusson · 2020-02-18T14:47:33Z

I'm not totally sure what test you want to see in TsExtractor.
I can add a testAitSample that has same code as testSample, on a 30kB sample which includes AIT, SDT, EIT and DSM-CC, does that good for you?

Those test data are defined using tsduck xml format. feedInputBuffer is identical to SpliceInfoDecoderTest's, could be worth mutualizing

icbaker · 2020-02-18T18:29:59Z

Looks good, thanks for making the changes! I'll work on getting this merged.

phhusson · 2020-02-27T18:13:07Z

Thanks!

Add minimal support of DVB AIT

1d65afd

This is used by broadcast channels to provide interactive contents.

googlebot added the cla: no label Jan 28, 2020

googlebot added cla: yes and removed cla: no labels Jan 28, 2020

ojw28 assigned icbaker Jan 30, 2020

icbaker requested changes Jan 31, 2020

View reviewed changes

icbaker added the pending comments label Feb 6, 2020

phhusson added 7 commits February 18, 2020 13:44

Remove dead-code readDvbString

7bf63e7

output is created at 'init', annotate it as lazyily initialized

79bc160

Comment SectionPassthrough

28c5043

[AitDecoder] Move to ParsableBitArray

98de7c4

[ParsableBitArray] Add readString

274743c

Use ParsableBitArray's readString

9060126

Rename variables to match standard, Add more comments refering to sta…

46e342b

…ndard

phhusson added 2 commits February 18, 2020 16:09

Include a new TS sample, with ait, dsm-cc, eit

8e84fe5

Include an AitDecoderTest

cfb2e9d

Those test data are defined using tsduck xml format. feedInputBuffer is identical to SpliceInfoDecoderTest's, could be worth mutualizing

icbaker added should merge and removed pending comments labels Feb 18, 2020

kim-vde merged commit 6946170 into google:dev-v2 Feb 27, 2020

phhusson deleted the feature/ait branch February 27, 2020 18:13

andrewlewis mentioned this pull request Apr 3, 2020

ExoPlayer fails to prepare for UHD MPEG-TS stream (worked before) #7177

Closed

google locked and limited conversation to collaborators Apr 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add minimal support of DVB AIT #6922

Add minimal support of DVB AIT #6922

phhusson commented Jan 28, 2020

googlebot commented Jan 28, 2020

phhusson commented Jan 28, 2020

googlebot commented Jan 28, 2020

phhusson commented Jan 28, 2020 •

edited

Loading

ojw28 commented Jan 30, 2020

icbaker left a comment

icbaker Jan 31, 2020

phhusson Feb 18, 2020

icbaker Jan 31, 2020

phhusson Feb 18, 2020

icbaker Jan 31, 2020

phhusson Feb 18, 2020

icbaker Jan 31, 2020

phhusson Feb 18, 2020

icbaker Jan 31, 2020

phhusson Feb 18, 2020

icbaker Jan 31, 2020

phhusson Feb 18, 2020

icbaker Jan 31, 2020

phhusson Feb 18, 2020

AquilesCanta Feb 18, 2020

phhusson Feb 18, 2020

phhusson commented Feb 18, 2020

phhusson commented Feb 18, 2020

icbaker commented Feb 18, 2020

phhusson commented Feb 27, 2020

Add minimal support of DVB AIT #6922

Add minimal support of DVB AIT #6922

Conversation

phhusson commented Jan 28, 2020

googlebot commented Jan 28, 2020

phhusson commented Jan 28, 2020

googlebot commented Jan 28, 2020

phhusson commented Jan 28, 2020 • edited Loading

ojw28 commented Jan 30, 2020

icbaker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phhusson commented Feb 18, 2020

phhusson commented Feb 18, 2020

icbaker commented Feb 18, 2020

phhusson commented Feb 27, 2020

phhusson commented Jan 28, 2020 •

edited

Loading