Replacing label with a list of Label elements in Format. #1054

jekopena · 2024-01-30T21:10:19Z

According to ISO/IEC 23009-1 [4], clause 5.3.7.2 Table 13, an AdaptationSet could include several Label elements (0..N), not only one. But right now Exoplayer only fetches the first Label and ignores the rest.

This PR fetches all the Labels and stores them in Format.labels that is a List of labels instead of only one label String. Also, instead of just storing a String, it creates the class Label that stores the three possible fields that can be related to the Label: id, lang and the value of the Label.

oceanjules · 2024-01-30T21:36:39Z

Hi @jekopena,

Thank you for this high-quality PR, your input is very appreciated.

Without delving much further into it, the immediate problem I see is that this looks like a breaking change. We tend to deprecate methods/fields first and remove them after a couple of releases to give people a chance to migrate. And Format is a very widely used class. In your case, I assume you'd want to deprecate setLabel(String s) and forward the call to setLabels(immutableList.of(s))

jekopena · 2024-01-30T21:44:04Z

Good point, let me keep label as deprecated and add labels.

tonihei · 2024-01-31T11:36:38Z

Could you describe in a bit more detail what you are trying to achieve? As @oceanjules noted above, this is quite a drastic change given how widely used this Format object is, so it's worth considering if and why this is really needed. As far as I can tell the 'label` field is already fully custom and could contain any String without following a specific contract. So, assuming you have to parse the output anyway based on your knowledge of what you expect, would it work to concatenate all labels together as a comma-separated string?

jekopena · 2024-01-31T19:36:11Z

@oceanjules I have updated the PR to keep label and mark it as deprecated and also adding labels. label is doing the same: store the adaptationSet label or the last <Label> element if there are any. And labels is a list that only keeps the list of <Label> elements with their corresponding id, lang and value. This shouldn't be a breaking change anymore.

@tonihei concatenating all labels in the current label field wouldn't work because that would break what current users are expecting from label, that is to only contain the adaptationSet label or the last of the <Label> elements.

What I'm trying to achieve is that a track with Director's commentary in English, for example, should contain a list of Labels with a description of the content in different languages. For example:

<AdaptationSet ... lang="en">
  ...
  <Label lang="en">Director's commentary</Label>
  <Label lang="de">Kommentar des Regisseurs</Label>
  <Label lang="es">Comentarios del Director</Label>
  ...
</AdaptationSet>

So the client can show a track description in the track selector depending on the language used by the client. If the client is using English, it can describe the track as "Director's commentary", or "Comentarios del Director" if it's using Spanish.

jekopena · 2024-02-07T16:23:14Z

@oceanjules @tonihei Hi! Do you have any more feedback about this PR?

icbaker · 2024-02-07T18:04:01Z

What I'm trying to achieve is that a track with Director's commentary in English, for example, should contain a list of Labels with a description of the content in different languages. For example:
<AdaptationSet ... lang="en">
  ...
  <Label lang="en">Director's commentary</Label>
  <Label lang="de">Kommentar des Regisseurs</Label>
  <Label lang="es">Comentarios del Director</Label>
  ...
</AdaptationSet>
So the client can show a track description in the track selector depending on the language used by the client. If the client is using English, it can describe the track as "Director's commentary", or "Comentarios del Director" if it's using Spanish.

If you're only adding a List<String> to the Format, how will consumers of this field access the lang part of your example?

jekopena · 2024-02-07T18:09:23Z

What I'm trying to achieve is that a track with Director's commentary in English, for example, should contain a list of Labels with a description of the content in different languages. For example:
<AdaptationSet ... lang="en">
  ...
  <Label lang="en">Director's commentary</Label>
  <Label lang="de">Kommentar des Regisseurs</Label>
  <Label lang="es">Comentarios del Director</Label>
  ...
</AdaptationSet>
So the client can show a track description in the track selector depending on the language used by the client. If the client is using English, it can describe the track as "Director's commentary", or "Comentarios del Director" if it's using Spanish.
If you're only adding a List<String> to the Format, how will consumers of this field access the lang part of your example?

I created a model called Label that contains the id, lang and value. And I'm not adding a List<String> to Format, it's a List<Label> so you would be able to access the lang from the Label.

icbaker · 2024-02-07T18:21:09Z

Ah right, sorry - read it too quickly and missed that.

tonihei · 2024-03-11T09:43:15Z

Sorry for the delay. It wasn't super clear to us yet how we think this should look like, but your change is definitely a sensible start. One of the issues is that it should not be too DASH specific, but rather represent a generic concept every streaming format could fill in if needed.

I was wondering if we really need the Label class for example. The main purpose at the moment is to add id and language fields to each String label. The language makes sense as differentiating element, but the id has a very specific grouping function in the DASH spec. I haven't seen an example where this is used, but it feels like the GroupLabel tag would translate to something like TrackGroup.label in ExoPlayer and the Format instances are already grouped together in the TrackGroup.

Is your goal to support the language tags or do you actually care about the group ids as well? If the main goal of your PR is the translation of the labels, then we could make this more explicit by adding for example a ImmutableMap<String, String> Format.localizedLabels, which also helps to describe the purpose of this field more clearly. If we then want to add support for label grouping as well, we can consider this separately.

jekopena · 2024-03-11T15:51:53Z

Hi @tonihei, yeah, I'm not interested in id either for my use case, I just added it because it was in the ISO spec but what you say seems logic since the different Formats are already grouped in the TrackGroup.

Your suggested ImmutableMap<String, String> in Format.localizedLabels looks good to me. Do you want me to change it in my PR?

jekopena · 2024-03-11T17:19:07Z

@tonihei by the way, lang is optional in <Label> per the ISO spec. So I don't think localizedLabels can be a Map using lang as a key. So maybe we could keep the Label class and localizedLabels to be a List<Label> but removing the id from Label?

tonihei · 2024-03-12T16:29:00Z

Thanks for all these useful comments! We discussed this a bit more and ended up with something similar to what you have already:

Use a named Label class to make it more future-proof even if the main use case for now is translation in other languages.
Do not add the id field as long as we don't know the exact use case. We can treat it as a separate feature request if this is required in the future. It would get a more descriptive name as well at this point (like groupId or similar).
Add the new Format.labels field, but do not deprecate the existing Format.label field. The existing field is still useful as a "default" label (which we should also point out in the Javadoc).
The point above means we should also make sure both fields can be relied upon by consumers of Format. This can be ensured in Format.Builder:
- if no labels are set, automatically populate them with a single entry matching label.
- if no label is set, automatically populate it by either the label matching Format.language or by the first one in the list.
- if both values are set, throw an lllegalStateException in Format.Builder.build() if labels doesn't contain label.

Overall, that should be pretty close to your existing pull request. Not deprecating the old field and having this default consistency update in Format.Builder also means you won't have to change many of the files touched in your current version. Do you want to make the remaining updates? We are happy to merge it with these changes.

jekopena · 2024-03-12T16:49:49Z

This sounds good to me, I'll start working in the changes!

jekopena · 2024-03-12T21:26:29Z

@tonihei working on this, I don't understand very well the lllegalStateException condition. After doing the first two points, we ensure that there will always be consistency if only one of them (label or labels) are set. It none are set, nothing changes, both will be empty. But if both are set in theory it should be possible to have a manifest that is setting a label that is not part of the <Label> list and I think we shouldn't throw an exception when this happens. If both are set, we should just leave them as they come from the manifest.

I pushed a commit with all the changes so you can take a look except the IllegalStateException part and without adding Format unit tests.

tonihei · 2024-03-13T08:15:09Z

it should be possible to have a manifest that is setting a label that is not part of the list and I think we shouldn't throw an exception when this happens

I was under the assumption that all labels are signalled via <Label> elements in the DASH manifest and there is no separate label that can be defined differently? If so, the situation you describe wouldn't happen, but I may be missing something. Could you explain where the label that is not part of the labels list comes from?

jekopena · 2024-03-13T15:22:51Z

@tonihei Correct, following the DASH manifest ISO, label shouldn't even exist. But currently it's implemented in Exoplayer because of a PR asking for it in 2018 (google/ExoPlayer#4391). Maybe <Label> list didn't exist by that time in the ISO? I don't know.

But I guess you want to keep label for legacy purposes, so clients that are currently relying on it, even when it's not part of the ISO, can keep using it. So thinking on these legacy clients not following ISO that are using label, I was thinking that they could have a legacy label beside a list of modern <Label> and not receiving an exception when they don't match. As for your answer, I guess maybe this is completely non sense.

So anyway, maybe I'm overthinking it, if you still want me to throw the lllegalStateException because a manifest will never set both label and <Label> list on its own, I'm ok doing it.

icbaker · 2024-03-13T15:59:54Z

But I guess you want to keep label for legacy purposes, so clients that are currently relying on it, even when it's not part of the ISO

A singular label also makes sense for other formats, e.g. Matroska, see Track > Label: https://www.matroska.org/technical/elements.html

The exception is nice because it means we can document Format.label as definitely being derived from/related to one of the entries in Format.labels. This means an app that wants to inspect all labels only has to look at Format.labels. Without this guarantee, we have to document things more vaguely, and apps are then obliged to check both Format.label and Format.labels and add some de-duping logic in case labels does contain label.

jekopena · 2024-03-13T17:53:51Z

@icbaker cool, I've added the IllegalStateException and some unit tests in FormatTest, so this should be ready for review.

tonihei

Thanks for making all these updates! I left a few comments in the code, nothing major, mostly adjusting to existing style and reusing utilities where they exist.

tonihei · 2024-03-14T13:46:07Z

gradle/wrapper/gradle-wrapper.properties

 distributionBase=GRADLE_USER_HOME
 distributionPath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-8.0-all.zip
+distributionUrl=https\://services.gradle.org/distributions/gradle-8.0-bin.zip


Could you revert this?

tonihei · 2024-03-14T13:49:10Z

libraries/common/src/main/java/androidx/media3/common/Format.java

+   * @return Whether the {@link #labels} belonging to this format and {@code other} are equal.
+   */
+  @UnstableApi
+  public boolean labelsEquals(Format other) {


I think labels.equals(other.labels) should work just fine. The List implementations override the equals method to do what this helper method is doing.

Sounds good, I'll delete it, I just created this one following the initializationDataEquals method.

tonihei · 2024-03-14T13:49:46Z

libraries/common/src/main/java/androidx/media3/common/Format.java

@@ -1111,7 +1161,8 @@ public int hashCode() {
      // Some fields for which hashing is expensive are deliberately omitted.
      int result = 17;
      result = 31 * result + (id == null ? 0 : id.hashCode());
-      result = 31 * result + (label != null ? label.hashCode() : 0);
+      result = 31 * result + (label == null ? 0 : label.hashCode());
+      // [Omitted] labels.


I think labels.hashCode() should work and there is no need to omit them.

tonihei · 2024-03-14T13:52:52Z

libraries/common/src/main/java/androidx/media3/common/Format.java

+    if (labels.isEmpty() && !TextUtils.isEmpty(tmpLabel)) {
+      labels.add(new Label(language, tmpLabel));
+    }
+    label = makeLabelIfNeeded(tmpLabel, labels);


To make the logic flow easier to read, could it add an else if (!labels.isEmpty() && tmpLabel == null) to call this method and rename the method to getDefaultLabel?

tonihei · 2024-03-14T13:54:54Z

libraries/common/src/main/java/androidx/media3/common/Format.java

@@ -1284,6 +1362,11 @@ public static String toLogString(@Nullable Format format) {
    if (format.label != null) {
      builder.append(", label=").append(format.label);
    }


Given the new guarantees that label and labels always match, we can now remove this part.

Which part? This code is just printing all the fields of the Format for logging, how is this affected by if label and labels match?

It will always print the default label twice now, which seems unnecessary for someone reading a log line to understand which Format is used.

Ok, I just removed the label from toLogString.

tonihei · 2024-03-14T13:56:12Z

libraries/common/src/main/java/androidx/media3/common/Format.java

@@ -1301,36 +1384,37 @@ public static String toLogString(@Nullable Format format) {

  private static final String FIELD_ID = Util.intToStringMaxRadix(0);
  private static final String FIELD_LABEL = Util.intToStringMaxRadix(1);
-  private static final String FIELD_LANGUAGE = Util.intToStringMaxRadix(2);


Could you leave all these numbers as they were and instead use the next available number? The reason is that these constants are used for serialization across processes and versions and they have to stay the same forever.

tonihei · 2024-03-14T14:21:21Z

libraries/common/src/main/java/androidx/media3/common/Format.java

@@ -1347,6 +1431,11 @@ public Bundle toBundle(boolean excludeMetadata) {
    Bundle bundle = new Bundle();
    bundle.putString(FIELD_ID, id);
    bundle.putString(FIELD_LABEL, label);
+    if (labels != null) {


To avoid this manual loop here, you can use putParcelableArrayList with toBundleArrayList, similar to

media/libraries/common/src/main/java/androidx/media3/common/MediaItem.java

Line 1293 in d13a0f4

bundle.putParcelableArrayList(

This will require toBundle/fromBundle methods in Label I believe.

jekopena · 2024-03-14T15:55:56Z

@tonihei I addressed your PR feedback except one I didn't understand. Feel free to resolve the conversations when you review the changes if you are ok with them!

tonihei · 2024-03-18T13:04:14Z

Thanks a lot! We'll work on merging the PR internally (you may see some additional commits for formatting fixes etc).

…t.Builder.

PiperOrigin-RevId: 619573181 (cherry picked from commit 8fe7033)

icbaker requested a review from tonihei February 7, 2024 18:04

icbaker assigned tonihei Feb 7, 2024

tonihei reviewed Mar 14, 2024

View reviewed changes

jekopena added 7 commits March 25, 2024 12:00

Replacing label with a list of Label elements in Format.

df76322

Reverting breaking changes by keeping label and adding labels.

63fb68e

Removing id from Label and ensuring label/labels consistency in Forma…

e9e47f4

…t.Builder.

Adding IllegalStateException to Format build and unit tests.

5910200

Addressing PR feedback.

0a04e79

Reverting changes in gradle-wrapper.properties

54c23e2

Removing label from toLogString.

8673e6d

tonihei force-pushed the main branch 4 times, most recently from cebd57e to e239ca4 Compare March 25, 2024 12:49

Additional clean-up and formatting

73c0ebb

tonihei force-pushed the main branch from e239ca4 to 73c0ebb Compare March 25, 2024 15:02

copybara-service bot merged commit 8fe7033 into androidx:main Mar 27, 2024
1 check passed

SheenaChhabra pushed a commit that referenced this pull request Apr 8, 2024

Merge pull request #1054 from jekopena:main

7b5522f

PiperOrigin-RevId: 619573181 (cherry picked from commit 8fe7033)

l1068 pushed a commit to l1068org/media that referenced this pull request Apr 15, 2024

Merge pull request androidx#1054 from jekopena:main

70f8ea2

PiperOrigin-RevId: 619573181 (cherry picked from commit 8fe7033)

androidx locked and limited conversation to collaborators May 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replacing label with a list of Label elements in Format. #1054

Replacing label with a list of Label elements in Format. #1054

jekopena commented Jan 30, 2024 •

edited

Loading

oceanjules commented Jan 30, 2024

jekopena commented Jan 30, 2024

tonihei commented Jan 31, 2024

jekopena commented Jan 31, 2024 •

edited

Loading

jekopena commented Feb 7, 2024

icbaker commented Feb 7, 2024

jekopena commented Feb 7, 2024

icbaker commented Feb 7, 2024

tonihei commented Mar 11, 2024

jekopena commented Mar 11, 2024

jekopena commented Mar 11, 2024

tonihei commented Mar 12, 2024 •

edited

Loading

jekopena commented Mar 12, 2024

jekopena commented Mar 12, 2024 •

edited

Loading

tonihei commented Mar 13, 2024

jekopena commented Mar 13, 2024

icbaker commented Mar 13, 2024

jekopena commented Mar 13, 2024

tonihei left a comment

tonihei Mar 14, 2024

tonihei Mar 14, 2024

jekopena Mar 14, 2024

tonihei Mar 14, 2024

tonihei Mar 14, 2024

tonihei Mar 14, 2024

jekopena Mar 14, 2024

tonihei Mar 18, 2024

jekopena Mar 18, 2024

tonihei Mar 14, 2024

tonihei Mar 14, 2024

jekopena commented Mar 14, 2024

tonihei commented Mar 18, 2024

Replacing label with a list of Label elements in Format. #1054

Replacing label with a list of Label elements in Format. #1054

Conversation

jekopena commented Jan 30, 2024 • edited Loading

oceanjules commented Jan 30, 2024

jekopena commented Jan 30, 2024

tonihei commented Jan 31, 2024

jekopena commented Jan 31, 2024 • edited Loading

jekopena commented Feb 7, 2024

icbaker commented Feb 7, 2024

jekopena commented Feb 7, 2024

icbaker commented Feb 7, 2024

tonihei commented Mar 11, 2024

jekopena commented Mar 11, 2024

jekopena commented Mar 11, 2024

tonihei commented Mar 12, 2024 • edited Loading

jekopena commented Mar 12, 2024

jekopena commented Mar 12, 2024 • edited Loading

tonihei commented Mar 13, 2024

jekopena commented Mar 13, 2024

icbaker commented Mar 13, 2024

jekopena commented Mar 13, 2024

tonihei 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

jekopena commented Mar 14, 2024

tonihei commented Mar 18, 2024

jekopena commented Jan 30, 2024 •

edited

Loading

jekopena commented Jan 31, 2024 •

edited

Loading

tonihei commented Mar 12, 2024 •

edited

Loading

jekopena commented Mar 12, 2024 •

edited

Loading