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

Fix access of mediaSrc value #6611

Conversation

jhonatangcavalcanti
Copy link
Contributor

This PR will...

Fix the access to retrieve the src value from current media.

The problem happens when there is a video element child which is not a source element. Other children could be track or any other element.

Now, instead of retrieving the src from firstChild, it is retrieved from the source element if it exists.

Why is this Pull Request needed?

The cleanup when detaching media is being skipped and an error is wrongly logged due to this problem.

Screenshot 2024-08-03 at 16 29 56

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

Resolves issues:

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

robwalch
robwalch previously approved these changes Aug 4, 2024
@robwalch robwalch added this to the 1.6.0 milestone Aug 4, 2024
@jhonatangcavalcanti
Copy link
Contributor Author

Hello, @robwalch. Is it possible to add this patch to a new version 1.5.4 on top of 1.5.3?

@robwalch
Copy link
Collaborator

robwalch commented Aug 4, 2024

Hello, @robwalch. Is it possible to add this patch to a new version 1.5.4 on top of 1.5.3?

Please open a PR against the "patch/v1.5.x" branch.

The latest release is v1.5.13 so that would get merged and released at v1.5.14.

Consider that there may be integrations or platforms where querySelector is not a method available on the object attached (not an actual HTMLMediaElement but an object with a src property).

@jhonatangcavalcanti jhonatangcavalcanti force-pushed the feature/fix-mediasrc-value branch from a12a6b5 to ac1c666 Compare August 4, 2024 23:23
@jhonatangcavalcanti jhonatangcavalcanti changed the base branch from master to patch/v1.5.x August 4, 2024 23:23
@jhonatangcavalcanti
Copy link
Contributor Author

Thanks, @robwalch. I did update this PR pointing to patch/v1.5.x.
I've also added the ac1c666 to keep compatibility with the current implementation of getting the mediaSrc from firstChild.

Is it ok?

@jhonatangcavalcanti
Copy link
Contributor Author

I'll need to update it again, the new implementation brings back the problem that I've reported.
I'll work on a new commit with a fix

@jhonatangcavalcanti jhonatangcavalcanti force-pushed the feature/fix-mediasrc-value branch from ac1c666 to 0013621 Compare August 4, 2024 23:35
@jhonatangcavalcanti
Copy link
Contributor Author

I updated and added a check to ensure that media.querySelector is available before executing it.

I don't know if there is a scenario where the media is not a video tag and the firstChild is a valid element with a src attribute.

@robwalch robwalch modified the milestones: 1.6.0, 1.5.14 Aug 5, 2024
src/controller/buffer-controller.ts Outdated Show resolved Hide resolved
@jhonatangcavalcanti jhonatangcavalcanti force-pushed the feature/fix-mediasrc-value branch from 13301b4 to 764551a Compare August 5, 2024 16:51
@robwalch robwalch merged commit 0d29c78 into video-dev:patch/v1.5.x Aug 5, 2024
10 checks passed
robwalch added a commit that referenced this pull request Aug 5, 2024
@robwalch
Copy link
Collaborator

robwalch commented Aug 5, 2024

@jhonatangcavalcanti,

Thank you for the fix. This has been released in v1.5.14 and merged into dev.

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