Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Descriptive audio is not selectable when the adaptation has a role of main, and ABR is enabled #4500

Closed
littlespex opened this issue Sep 21, 2022 · 6 comments · Fixed by #4629 or #4424
Labels
priority: P1 Big impact or workaround impractical; resolve before feature release status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Milestone

Comments

@littlespex
Copy link
Contributor

littlespex commented Sep 21, 2022

Have you read the FAQ and checked for duplicate open issues?
Yes

What version of Shaka Player are you using?
3.x, 4.x

Can you reproduce the issue with our latest release version?
Yes

Can you reproduce the issue with the latest code from main?
Yes

Are you using the demo app or your own custom app?
Both

If custom app, can you reproduce the issue using our demo app?
Yes

What browser and OS are you using?
macOS 12.6, Chrome 105.0.5195.125

For embedded devices (smart TVs, etc.), what model and firmware version are you using?

What are the manifest and license server URIs?

Sent separately

What configuration are you using? What is the output of player.getConfiguration()?

Default configuration

What did you do?

Load a stream with two audio tracks, both English (en-US), one with a role of main and one with a role of description. When playback begins switch to the description track using player.selectAudioLanguage('en-US', 'description').

What did you expect to happen?
Player switches tracks, descriptive audio can be heard, and video ABR performs as expected.

What actually happened?

When ABR is enabled (player.configure({ abr: { enabled: true } })), the track is selected, but is then shortly changed as the ABR algorithm selects a different bitrate. The following warning appears in the console, and no descriptive audio can be heard:

player.js:3819 Changing tracks while abr manager is enabled will likely result in the selected track being overriden. Consider disabling abr before calling selectVariantTrack().

When ABR is disabled (player.configure({ abr: { enabled: false } })), the descriptive audio can be heard, but all ABR functionality is lost and the player remains at the last bitrate for the remainder of the stream.

@littlespex littlespex added the type: bug Something isn't working correctly label Sep 21, 2022
@github-actions github-actions bot added this to the v4.3 milestone Sep 21, 2022
@littlespex
Copy link
Contributor Author

littlespex commented Sep 22, 2022

I'm not sure if this is relevant or not, but when player.getVariantTracks() is called, it returns that seems like it has too many items. There are 6 video representations and 2 audio representation which I think should only produce 12 variants, but it returns 30. Additionally, some of the variants in the list have an audioRoles property that contains both main and description, even though the manifest only defines one role per audio adaptation set.

4500

@littlespex
Copy link
Contributor Author

littlespex commented Sep 22, 2022

@joeyparrish Looking at the period combination code, when an audio stream with a single role of main is compared to another with a single role of description, they are considered compatible, and this results in the large list seen in the screenshot above. When I modify the areAVStreamsCompatible_ function to additionally compare roles, the issue goes away, and the variant list produces the 12 items I think it should have:

  /**
   * @param {T} outputStream An audio or video output stream
   * @param {T} candidate A candidate stream to be combined with the output
   * @return {boolean} True if the candidate could be combined with the
   *   output stream
   *
   * @template T
   * Accepts either a StreamDB or Stream type.
   *
   * @private
   */
  static areAVStreamsCompatible_(outputStream, candidate) {
    const getCodec = (codecs) =>
      shaka.util.MimeUtils.getNormalizedCodec(codecs);
    // Check MIME type and codecs, which should always be the same.
    if (candidate.mimeType != outputStream.mimeType ||
        getCodec(candidate.codecs) != getCodec(outputStream.codecs)) {
      return false;
    }

    /////////////////////////////////////////////////////////////////////
    // NOTE: This is an overly simplified way to compare the roles lists
    //       and is being used just to demostrate the idea of adding role
    //       comparison to the compatibility check.
    /////////////////////////////////////////////////////////////////////
    if (outputStream.type === 'audio') {
      if (outputStream.roles.join('') !== candidate.roles.join('')) {
        return false;
      }
    }
    /////////////////////////////////////////////////////////////////////

    // This field is only available on Stream, not StreamDB.
    if (outputStream.drmInfos) {
      // Check for compatible DRM systems.  Note that clear streams are
      // implicitly compatible with any DRM and with each other.
      if (!shaka.media.DrmEngine.areDrmCompatible(outputStream.drmInfos,
          candidate.drmInfos)) {
        return false;
      }
    }

    return true;
  }

While the code above is a quick hack, is the concept of adding role comparison to the compatibility routine a sound approach?

@joeyparrish
Copy link
Member

The concept is reasonable. The PeriodCombiner tests follow a simple pattern of "input" => "expected output", IIRC. Please check your changes there with a new case that is similar to the content that's failing.

I would also caution you that this should still work:

period 0 period 1 (ad) period 2
[main, description] [main] [main, description]

output: [main+main+main, description+main+description]

If an ad period is missing description tracks, we don't want to fail to find a valid combination. So perhaps you need to treat roles as a preference, but not a hard requirement.

@littlespex
Copy link
Contributor Author

littlespex commented Sep 22, 2022

Another piece of the puzzle: If the <Role value="main" /> nodes are removed from the audio AdaptationSets, the issue goes away and we see 12 variant tracks as expected:

player.getNetworkingEngine().registerResponseFilter((type, response) => {
  if (type !== shaka.net.NetworkingEngine.RequestType.MANIFEST) {
    return;
  }

  const decoder = new TextDecoder();
  const parser = new DOMParser();
  const xml = parser.parseFromString(decoder.decode(response.data), "text/xml");
  const roles = xml.querySelectorAll('AdaptationSet[contentType=audio] Role[value=main]');
  roles.forEach(role => role.parentElement.removeChild(role));

  const encoder = new TextEncoder();
  response.data = encoder.encode(new XMLSerializer().serializeToString(xml));
});

@littlespex littlespex changed the title Descriptive audio is not selectable when ABR is enabled Descriptive audio is not selectable when the adaptation has a role of main, and ABR is enabled Sep 23, 2022
@littlespex
Copy link
Contributor Author

littlespex commented Sep 23, 2022

When the main role exists on the audio adaptation set, the corresponding audio stream gets flagged as primary. The combination of primary and non-primary streams seems to be source of the issue. Here is a valid test that fails in the latest code base:

it('Matches streams with primary in common', async () => {
  const makeAudioStreamWithRoles = (id, roles, primary = true) => {
    const stream = makeAudioStream('en');
    stream.originalId = id;
    stream.roles = roles;
    stream.primary = primary;
    return stream;
  };

  /** @type {!Array.<shaka.util.PeriodCombiner.Period>} */
  const periods = [
    {
      id: '1',
      videoStreams: [
        makeVideoStream(720),
        makeVideoStream(1080),
      ],
      audioStreams: [
        // stream1,
        // stream2,
        makeAudioStreamWithRoles('stream1', ['main']),
        makeAudioStreamWithRoles('stream2', ['description'], false),
      ],
      textStreams: [],
      imageStreams: [],
    },
    {
      id: '2',
      videoStreams: [
        makeVideoStream(720),
        makeVideoStream(1080),
      ],
      audioStreams: [
        // stream3,
        // stream4,
        makeAudioStreamWithRoles('stream1', ['main']),
        makeAudioStreamWithRoles('stream2', ['description'], false),
      ],
      textStreams: [],
      imageStreams: [],
    },
  ];

  await combiner.combinePeriods(periods, /* isDynamic= */ false);
  const variants = combiner.getVariants();

  console.log(variants);

  expect(variants.length).toBe(4);

  expect(variants).toEqual(jasmine.arrayWithExactContents([
    makeAVVariant(720, 'en'),
    makeAVVariant(1080, 'en'),
    makeAVVariant(720, 'en'),
    makeAVVariant(1080, 'en'),
  ]));

  // We can use the originalId field to see what each track is composed of.
  const audio1 = variants[0].audio;
  expect(audio1.roles).toEqual(['main']);
  expect(audio1.originalId).toBe('stream1,stream1');

  const audio2 = variants[1].audio;
  expect(audio2.roles).toEqual(['main']);
  expect(audio2.originalId).toBe('stream1,stream1');

  const audio3 = variants[2].audio;
  expect(audio3.roles).toEqual(['description']);
  expect(audio3.originalId).toBe('stream2,stream2');

  const audio4 = variants[3].audio;
  expect(audio4.roles).toEqual(['description']);
  expect(audio4.originalId).toBe('stream2,stream2');
});

@littlespex
Copy link
Contributor Author

littlespex commented Sep 30, 2022

@joeyparrish The test above illustrates a very straight forward use case that the period flattening does not process correctly:

  • Two audio AdaptationSets in each period, one flagged as main, the other description, both with a language of en-US.
  • The IDs for the audio AdaptationSets are the same across each period.

The resulting variant track list contains variants with mixed main and description audio. Since there are valid adaptations for each role in every period, each variant should have only main, or only description. The issue is that the mixed main/description variants are being selected by the ABR decisioning, which results in the description audio being lost after player.selectAudioLanguage('en-US', 'description') is called.

I've tried a few different approaches in PeriodCombiner to resolve this, but it has turned into a game of whack-a-mole, where a fix for this test breaks others. I've sent the streams via email as well. Is there any insight you can provide to get the period flatten to work so that only valid variants are returned? (there should be no mixing of main and description).

@avelad avelad added the priority: P1 Big impact or workaround impractical; resolve before feature release label Oct 27, 2022
avelad pushed a commit that referenced this issue Oct 28, 2022
Perform the `primary` check after `role` check to improve period
flattening on streams with both `main` and `description` audio tracks.

Resolves #4500

Co-authored-by: Dan Sparacio <[email protected]>
joeyparrish pushed a commit that referenced this issue Oct 28, 2022
Perform the `primary` check after `role` check to improve period
flattening on streams with both `main` and `description` audio tracks.

Resolves #4500

Co-authored-by: Dan Sparacio <[email protected]>
joeyparrish pushed a commit that referenced this issue Oct 28, 2022
Perform the `primary` check after `role` check to improve period
flattening on streams with both `main` and `description` audio tracks.

Resolves #4500

Co-authored-by: Dan Sparacio <[email protected]>
joeyparrish pushed a commit that referenced this issue Oct 28, 2022
Perform the `primary` check after `role` check to improve period
flattening on streams with both `main` and `description` audio tracks.

Resolves #4500

Co-authored-by: Dan Sparacio <[email protected]>
joeyparrish pushed a commit that referenced this issue Oct 28, 2022
Perform the `primary` check after `role` check to improve period
flattening on streams with both `main` and `description` audio tracks.

Resolves #4500

Co-authored-by: Dan Sparacio <[email protected]>
joeyparrish pushed a commit that referenced this issue Oct 29, 2022
Perform the `primary` check after `role` check to improve period
flattening on streams with both `main` and `description` audio tracks.

Resolves #4500

Co-authored-by: Dan Sparacio <[email protected]>
joeyparrish pushed a commit that referenced this issue Oct 29, 2022
Perform the `primary` check after `role` check to improve period
flattening on streams with both `main` and `description` audio tracks.

Resolves #4500

Co-authored-by: Dan Sparacio <[email protected]>
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Dec 27, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
priority: P1 Big impact or workaround impractical; resolve before feature release status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
3 participants