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

Bugfix/ID3 Decoding On Play Station 4 #6042

Merged
merged 1 commit into from
Dec 16, 2023

Conversation

agajassi
Copy link
Contributor

This PR will...

On PS4, TextDecoder is defined but it is partially implemented and does not function properly. This will force manual decoding approach for PS4 platform in utf8ArrayToStr function.

Why is this Pull Request needed?

It is necessary for private id3s to be correctly decoded and converted on Play Station 4 platform.

Are there any points in the code the reviewer needs to double check?

Resolves issues: #6041

Checklist

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • API or design changes are documented in API.md

On PS4 TextDecoder is defined but it is partially implemented and does not function properly. This will force manual decoding approach for PS4 platform.
Comment on lines 397 to 408
let decoder: TextDecoder;

function getTextDecoder() {
// On Play Station 4, TextDecoder is defined but partially implemented.
// Manual decoding option is preferable
if (navigator.userAgent.includes('PlayStation 4')) {
return;
}

if (!decoder && typeof self.TextDecoder !== 'undefined') {
decoder = new self.TextDecoder('utf-8');
}
Copy link
Collaborator

@robwalch robwalch Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be a good candidate for feature detection? Rather than check userAgent, can we do a check the first time getTextDecoder() is called and set a flag like supportsUtf8Decoding?

Can you work out what byte(s) would make for such a test? Here's an example that checks decoded length. It assumes the decoder can be reused (might need to create a new one for the test).

let supportsUtf8Decoding?: boolean;
let decoder: TextDecoder;

function getTextDecoder() {
  if (supportsUtf8Decoding === false) {
    return;
  }
  if (!decoder && typeof self.TextDecoder !== 'undefined') {
    decoder = new self.TextDecoder('utf-8');
  }
  if (supportsUtf8Decoding === undefined) {
    // not sure what the test should look like
    if (decoder.decode(new Uint8Array([0xf9])).length === 0) {
      supportsUtf8Decoding = false;
      decoder = undefined;
      return;
    }
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robwalch I tried various set of byte sequences as a test. I keep getting inconsistent results every time I run it on PS4. It will evaluate as true and if you reload the page, second time it will be false. So far I cannot come up with a reliable byte sequence to use for feature testing.

I tried these:

  • '€'
    new Uint8Array([0xE2, 0x82, 0xAC]);
  • 'testñtest'
    new Uint8Array([0x74, 0x65, 0x73, 0x74, 0xC3, 0xB1, 0x74, 0x65, 0x73, 0x74]);
  • 'ID3......?'
    new Uint8Array([73, 68, 51, 4, 0, 0, 0, 0, 0, 63])
  • 'Hello é€😀'
    new Uint8Array([0x48, 0x65, 0x6C, 0x6C, 0x6F, 0x20, 0xC3, 0xA9, 0xE2, 0x82, 0xAC, 0xF0, 0x9F, 0x98, 0x80]);

As of now I have not seen this type of issue on any other devices (and I work and test with so many set top boxes and tvs). To me trying to make this generic by using feature detection does not seem to be beneficial.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for trying and going down the rabbit hole on this.

I see the UA workaround in other MSE players so let's go with it!

@robwalch robwalch added this to the 1.5.0 milestone Dec 14, 2023
@robwalch
Copy link
Collaborator

For this to go into 1.4.14, we need the change in patch/v1.4.x. After this is approved and merged, please cherry-pick into that branch and open another PR.

@robwalch robwalch merged commit b7aaa0a into video-dev:master Dec 16, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants