Parse role and accessibility descriptor values from adaptation sets by szaboa · Pull Request #5578 · google/ExoPlayer

szaboa · 2019-02-27T21:57:34Z

Pull request for Issue #5529 .

…format

ojw28 · 2019-03-03T14:19:56Z

...ash/src/main/java/com/google/android/exoplayer2/source/dash/manifest/DashManifestParser.java

    return MimeTypes.AUDIO_E_AC3;
  }

+  protected static String parseRole(List<Descriptor> roleDescriptors) {


Making these non-static to allow overriding seems fine to me. And move them up to be grouped with the other protected non-static methods.

Note that after the suggestions I made on the corresponding issue, these methods will return @Role and @Accessibility integers respectively.

I'd be tempted to make a parseSelectionFlags method too, that can parse selection flags from the role descriptors. The default implementation will contain the little piece of code that decides whether to set C.SELECTION_FLAG_DEFAULT that's currently further up in this class.

ojw28 · 2019-03-03T14:36:53Z

library/core/src/main/java/com/google/android/exoplayer2/Format.java

+  /**
+   * The Role descriptor value.
+   */
+  public final String role;


Note that video formats can have role/accessibility as well. As can selection flags actually. So these three params shouldn't be under "// Audio and text specific.". They should probably be right up under the label param, or somewhere near there.

Then I guess the parsing also needs to be moved up, right now it is called only in case of mimeTypeIsRawText(sampleMimeType) == true in DashManifestParser. Right?

… also for video / audio tracks

szaboa · 2019-03-03T18:28:09Z

Updated.

ojw28 · 2019-03-04T08:05:43Z

library/core/src/main/java/com/google/android/exoplayer2/C.java

+   */
+  @Documented
+  @Retention(RetentionPolicy.SOURCE)
+  @IntDef({


As per the issue thread, I'd suggest merging the two IntDefs and making it flag = true. You can then also remove the UNSET constants, since unset is just 0 for flags.

ojw28 · 2019-03-04T08:06:21Z

library/core/src/main/java/com/google/android/exoplayer2/Format.java

+  public final int selectionFlags;
+  /** Track role descriptor value, or {@link C#ROLE_UNSET} if unknown or not applicable. **/
+  @C.Role
+  public final int role;


And having a single roleFlags, rather than separate role and accessibility fields.

ojw28 · 2019-03-04T08:09:52Z

...ash/src/main/java/com/google/android/exoplayer2/source/dash/manifest/DashManifestParser.java

+    return C.ROLE_UNSET;
+  }
+
+  protected @C.Accessibility int parseAccessibility(List<Descriptor> accessibilityDescriptors) {


If going with my suggested approach, this method will return C.RoleFlags, which is slightly strange. But then it's somewhat strange DASH-IF separated use of urn:mpeg:dash:role:2011 across Role and Accessibility elements to start with, in my opinion :)... They should then be OR'd with the parseRole result to get the final flags.

You could probably have a single method parse "urn:mpeg:dash:role:2011" that you call from both parseRole and parseAccessibility. I think we'd probably want to handle things like "description" appearing as Role elements as well.

You could probably have a single method parse "urn:mpeg💨role:2011" that you call from both parseRole and parseAccessibility. I think we'd probably want to handle things like "description" appearing as Role elements as well.

This would mean that the roleFlags would have a common flag for role's "description" and accessibility's "description" values. In this case we couldn't decide if the "description" came from role or from accessibility.

Even if a couple of values could be the same for role and accessibility, I would store them as separate flags, as at the end we need to know where a value comes from (role or accessibility descriptor). Or am I missing something?

Nevermind, probably we can merge these as you said under the correspondent issue.

szaboa · 2019-03-05T21:40:33Z

Merged role and accessibility fields together as you've suggested.

ojw28 · 2019-03-06T00:53:54Z

library/core/src/main/java/com/google/android/exoplayer2/C.java

  public static final int NETWORK_TYPE_OTHER = 8;

+  /**
+   * Adaptation set's role and accessibility descriptor value.


nit - Avoid DASH specific terms; there's no reason roles can't be used for other media formats. I would also avoid mentioning accessibility; since outside of the DASH specific parsing where they're merged, they're just "roles" and nothing else. Would suggest documenting like:

Track role flags. Possible values are {@link XXX}, ...

See how SelectionFlags is documented for an example.

ojw28

Looking good. Just a bunch of nitty little things really, and then we'll get this merged. Thanks!

ojw28 · 2019-03-06T00:54:33Z

library/core/src/main/java/com/google/android/exoplayer2/C.java

+  })
+  public @interface RoleFlags {}
+
+  public static final int ROLE_FLAGS_MAIN = 1;


Can we please add Javadoc on each of these constants?

Yeah, I wanted but I didn't find the standard you were referred to (draft of third version of 23009-1) and I wanted to take the definition from there. In the second edition fo 23009-1 a couple of role values are missing (e.g. "sign"). But I will search for it again.

Feel free to leave them if you don't have the doc. I can add in some documentation when I merge.

I haven't found it yet, please add these comments.

ojw28 · 2019-03-06T00:55:39Z

library/core/src/main/java/com/google/android/exoplayer2/Format.java

+  /** Track selection flags. **/
+  @C.SelectionFlags
+  public final int selectionFlags;
+  /** Track role and accessibility descriptor values. **/


nit: Just "Track role flags."

ojw28 · 2019-03-06T00:56:51Z

library/core/src/main/java/com/google/android/exoplayer2/Format.java

      @C.SelectionFlags int selectionFlags,
-      @Nullable String language) {
+      @Nullable String language,
+      @C.RoleFlags int roleFlags) {


nit: Seems nice to group selectionFlags and roleFlags together, as in the methods above? Ditto for methods below this one.

ojw28 · 2019-03-06T00:58:02Z

library/core/src/main/java/com/google/android/exoplayer2/Format.java

      @C.SelectionFlags int selectionFlags,
      @Nullable String language,
      int accessibilityChannel,
+      @C.RoleFlags int roleFlags,


nit: Put next to selectionFlags? Ditto x lots below.

ojw28 · 2019-03-06T01:02:25Z

...ash/src/main/java/com/google/android/exoplayer2/source/dash/manifest/DashManifestParser.java

      } else if (XmlPullParserUtil.isStartTag(xpp, "Role")) {
-        selectionFlags |= parseRole(xpp);
+        Descriptor descriptor = parseDescriptor(xpp, "Role");
+        selectionFlags |= parseSelectionFlags(descriptor);


It seems cleaner to have a parseSelection(roleDescriptors) that you can call in buildFormat. This saves you from having to pass selectionFlags into that method separately.

ojw28 · 2019-03-06T01:05:10Z

...ash/src/main/java/com/google/android/exoplayer2/source/dash/manifest/DashManifestParser.java

            language,
-            accessibilityChannel);
+            accessibilityChannel,
+            roleFlags);


I think you probably need to pass roleFlags into the Format.createContainerFormat on L647 (below) as well as the ones you've already done.

ojw28 · 2019-03-06T01:08:28Z

...ash/src/main/java/com/google/android/exoplayer2/source/dash/manifest/DashManifestParser.java

+      if ("urn:mpeg:dash:role:2011".equalsIgnoreCase(descriptor.schemeIdUri) && descriptor.value != null) {
+        result |= parseRoleSchemeValue(descriptor.value);
+      }
+      if ("urn:tva:metadata:cs:AudioPurposeCS:2007".equalsIgnoreCase(descriptor.schemeIdUri) &&


nit: Use else if

ojw28 · 2019-03-06T01:09:46Z

...ash/src/main/java/com/google/android/exoplayer2/source/dash/manifest/DashManifestParser.java

+      }
+      if ("urn:tva:metadata:cs:AudioPurposeCS:2007".equalsIgnoreCase(descriptor.schemeIdUri) &&
+              descriptor.value != null) {
+        switch (descriptor.value){


Although it's only used here, it might be nice to split this into a parseAudioPurposeValue method, for consistency with parseRoleSchemeValue.

ojw28 · 2019-03-06T01:10:41Z

...ash/src/main/java/com/google/android/exoplayer2/source/dash/manifest/DashManifestParser.java

+    @C.RoleFlags int result = 0;
+    switch (value) {
+      case "main":
+        result |= C.ROLE_FLAGS_MAIN;


You can just return C.ROLE_FLAGS_X; for each case, and add a default case that returns 0. Ditto for parseAudioPurposeValue if you make that a method too.

ojw28 · 2019-03-06T01:12:33Z

...ash/src/main/java/com/google/android/exoplayer2/source/dash/manifest/DashManifestParser.java

+    @C.RoleFlags int result = 0;
+    for (int i = 0; i < roleDescriptors.size(); i++) {
+      Descriptor descriptor = roleDescriptors.get(i);
+      if ("urn:mpeg:dash:role:2011".equalsIgnoreCase(descriptor.schemeIdUri) && descriptor.value != null) {


It would IMO be neater for the null check to happen at the start of parseRoleSchemeValue (ditto parseAudioPurposeValue if you make that).

szaboa · 2019-03-06T09:43:43Z

I will do these minor changes later today. You would like to have some test cases for this whole role flags, or do you think isn't so important right now?

ojw28 · 2019-03-06T10:09:00Z

Thanks! Feel free to add some test cases if you think they'd be helpful, but I don't think it's hugely important in this case. The non-parser parts are trivial, and parsing code tends to be write-once-never-touch-again.

szaboa · 2019-03-06T22:12:39Z

Updated. I hope I've covered everything, please say so if not. I decided to not to add tests for now :)

ojw28 · 2019-03-06T22:48:42Z

Thanks!

This is a precursor to merging #5578 PiperOrigin-RevId: 239371046

PiperOrigin-RevId: 239398940

Pass role and accessibility descriptor values from adaptation set to …

06ed877

…format

googlebot added the cla: yes label Feb 27, 2019

Remove old parseRole method that looked only for main

43ea388

ojw28 self-assigned this Mar 3, 2019

ojw28 reviewed Mar 3, 2019

View reviewed changes

Add IntDef for role and accessibility descriptor's value, parse these…

58d746e

… also for video / audio tracks

ojw28 reviewed Mar 4, 2019

View reviewed changes

Merge role and accessibility fields into RoleFlags

8544199

ojw28 reviewed Mar 6, 2019

View reviewed changes

Fixes according to the code review

d7c2519

ojw28 added the should merge label Mar 6, 2019

ojw28 merged commit d7c2519 into google:dev-v2 Mar 20, 2019

ojw28 added a commit that referenced this pull request Mar 20, 2019

Sanitize internal parameter ordering in Format

448ca0c

This is a precursor to merging #5578 PiperOrigin-RevId: 239371046

ojw28 added a commit that referenced this pull request Mar 20, 2019

Merge pull request #5578 from szaboa:dev-v2-5529

a86a913

PiperOrigin-RevId: 239398940

szaboa mentioned this pull request Mar 23, 2019

AudioPurpose parsing of subtitle track from DASH manifest #5529

Closed

google locked and limited conversation to collaborators Aug 5, 2019

Conversation

szaboa commented Feb 27, 2019

Uh oh!

ojw28 Mar 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szaboa commented Mar 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szaboa Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szaboa Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szaboa commented Mar 5, 2019

Uh oh!

ojw28 Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ojw28 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szaboa commented Mar 6, 2019

Uh oh!

ojw28 commented Mar 6, 2019

Uh oh!

szaboa commented Mar 6, 2019

Uh oh!

ojw28 commented Mar 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ojw28 Mar 3, 2019 •

edited

Loading

szaboa Mar 5, 2019 •

edited

Loading

szaboa Mar 5, 2019 •

edited

Loading

ojw28 Mar 6, 2019 •

edited

Loading