audioStreamFormat gets incorrect id from generate_ids

[magnus-nomono]

Currently generate_ids has the following code for audioStreamFormat ids:

for id, element in enumerate(non_common(adm.audioStreamFormats), 0x1001):
    element.id = "AS_{format.value:04X}{id:04X}".format(id=id, format=element.format)

    for track_id, element in enumerate(_stream_track_formats(adm, element), 0x1):
        element.id = "AT_{format.value:04X}{id:04X}_{track_id:02X}".format(id=id, format=element.format, track_id=track_id)

According to the definition the yyyy part of the id should reflect the content type not the format. Since audioStreamFormat does not contain its own type property, we should use the type of the parent audioPackFormat/audioChannelFormat. Something like this maybe:

for id, element in enumerate(non_common(adm.audioStreamFormats), 0x1001):
    if element.audioChannelFormat:
        content_type = element.audioChannelFormat.type
    elif element.audioPackFormat:
        content_type = element.audioPackFormat.type
    else:
        raise ValueError("audioStreamFormat needs to reference an audioChannelFormat or audioPackFormat")

    element.id = "AS_{type.value:04X}{id:04X}".format(id=id, type=content_type)

    for track_id, element in enumerate(_stream_track_formats(adm, element), 0x1):
        element.id = "AT_{type.value:04X}{id:04X}_{track_id:02X}".format(id=id, type=content_type, track_id=track_id)

Changing some of the data structures might render a better solution

[tomjnixon]

Thank you, I agree. Arguably the wording is ambiguous, but the examples agree with you.

I'd rather not change the data structures if not necessary; as they are they mostly match the ADM, and this information is not required anywhere else as far as i know.

[magnus-nomono]

Thank you, I agree. Arguably the wording is ambiguous, but the examples agree with you.

Yes, it is also the way I have seen it interpreted elsewhere

I'd rather not change the data structures if not necessary; as they are they mostly match the ADM, and this information is not required anywhere else as far as i know.

Thinking some more about I agree that adding the type to audioStreamFormat is probably not a good solution. A potential issue with the above solution is that currently audioStreamFormat does not require to link to either an audioChannelFormat or an audioPackFormat. Arguably it does not make much sense to set neither of them, so if there is a clear error message it should probably be fine to have this requirement only in generate_ids, although ideally it would be in audioStreamFormat itself

[tomjnixon]

Arguably it does not make much sense to set neither of them, so if there is a clear error message it should probably be fine to have this requirement only in generate_ids, although ideally it would be in audioStreamFormat itself

Yes. This is currently checked in the ASF validate method:

https://github.com/ebu/ebu_adm_renderer/blob/a04e287f247809653a031b7c99306066ec3d94c1/ear/fileio/adm/elements/main_elements.py#L467-L469

In general it is a bit tricky to do this kind of thing properly (i.e. making invalid states unrepresentable), because it makes it too dificult to construct or modify the data structures in an imperative manner (without transitioning through invalid states).

This unfortunately means that validation (or at least dealing with invalid data in a somewhat graceful way) has to happen in places like this. There are quite a lot of assertions covering this kind of thing, as it's a programming error to create invalid data, or to not run validation on parsed data, and therefore errors of this type should never be caught.

Anyway, i'll try to make time to fix this in the next couple of days.