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))

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

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?

@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.

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

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?

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.

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

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.

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?

@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?

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.

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

@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.

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?

@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.

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.

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

@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!

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