Skip to content

Conversation

@jernoble
Copy link
Contributor

@jernoble jernoble commented Jan 10, 2025

e3ec66b

[Cocoa][MSE] Seeking to the end of X.com videos will never complete
https://bugs.webkit.org/show_bug.cgi?id=285776
rdar://136845120

Reviewed by Jean-Yves Avenard.

We already allow selections immediately before a buffered range to succeed; also
apply the same "fudge factor" to allow seeking just past the end of a buffered range.

* LayoutTests/media/media-source-seek-past-end-expected.txt: Added.
* LayoutTests/media/media-source-seek-past-end.html: Added.
* Source/WebCore/platform/graphics/TrackBuffer.cpp:

Canonical link: https://commits.webkit.org/288962@main

912e521

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ❌ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt loading-orange 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@jernoble jernoble self-assigned this Jan 10, 2025
@jernoble jernoble added the Media Bugs related to the HTML 5 Media elements. label Jan 10, 2025
@jernoble jernoble force-pushed the eng/Cocoa-MSE-Seeking-to-the-end-of-X-com-videos-will-never-complete branch from e5b9bfa to d96fedf Compare January 10, 2025 23:57
@jernoble jernoble requested a review from a team January 10, 2025 23:57
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 11, 2025
@jernoble jernoble removed the merging-blocked Applied to prevent a change from being merged label Jan 11, 2025
@jernoble jernoble force-pushed the eng/Cocoa-MSE-Seeking-to-the-end-of-X-com-videos-will-never-complete branch from d96fedf to 4268d6d Compare January 11, 2025 02:34
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 11, 2025
@jernoble jernoble removed the merging-blocked Applied to prevent a change from being merged label Jan 13, 2025
@jernoble jernoble force-pushed the eng/Cocoa-MSE-Seeking-to-the-end-of-X-com-videos-will-never-complete branch from 4268d6d to 75f6f07 Compare January 13, 2025 17:48
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 13, 2025
@jernoble jernoble removed the merging-blocked Applied to prevent a change from being merged label Jan 13, 2025
@jernoble jernoble force-pushed the eng/Cocoa-MSE-Seeking-to-the-end-of-X-com-videos-will-never-complete branch from 75f6f07 to ec8a364 Compare January 13, 2025 20:51
Copy link
Member

Choose a reason for hiding this comment

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

I think this won't be sufficient for webm; Ref<GenericPromise> MediaPlayerPrivateWebM::seekTo(const MediaTime& time) will not complete the seek if it's not within the buffered range, and the duration won't be.

If we want to handle this case, needs to be done there.

Copy link
Member

Choose a reason for hiding this comment

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

Is this needed here?

For the issue at and, below is all necessary right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just refactoring the existing check.

Copy link
Member

Choose a reason for hiding this comment

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

Would be nice if the test checked that seeking past the duration won't complete if the source isn't ended.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 13, 2025
@jernoble jernoble removed the merging-blocked Applied to prevent a change from being merged label Jan 14, 2025
@jernoble jernoble force-pushed the eng/Cocoa-MSE-Seeking-to-the-end-of-X-com-videos-will-never-complete branch from ec8a364 to 912e521 Compare January 14, 2025 21:00
@jernoble jernoble added the merge-queue Applied to send a pull request to merge-queue label Jan 15, 2025
@webkit-commit-queue
Copy link
Collaborator

Commit message contains (OOPS!) and no valid reviewer found, blocking PR #38874. Details: Build #18226

@webkit-commit-queue webkit-commit-queue added merging-blocked Applied to prevent a change from being merged and removed merge-queue Applied to send a pull request to merge-queue labels Jan 15, 2025
@jyavenard jyavenard added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Jan 15, 2025
https://bugs.webkit.org/show_bug.cgi?id=285776
rdar://136845120

Reviewed by Jean-Yves Avenard.

We already allow selections immediately before a buffered range to succeed; also
apply the same "fudge factor" to allow seeking just past the end of a buffered range.

* LayoutTests/media/media-source-seek-past-end-expected.txt: Added.
* LayoutTests/media/media-source-seek-past-end.html: Added.
* Source/WebCore/platform/graphics/TrackBuffer.cpp:

Canonical link: https://commits.webkit.org/288962@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Cocoa-MSE-Seeking-to-the-end-of-X-com-videos-will-never-complete branch from 912e521 to e3ec66b Compare January 15, 2025 21:14
@webkit-commit-queue
Copy link
Collaborator

Committed 288962@main (e3ec66b): https://commits.webkit.org/288962@main

Reviewed commits have been landed. Closing PR #38874 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit e3ec66b into WebKit:main Jan 15, 2025
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Media Bugs related to the HTML 5 Media elements.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants