fix(service-worker): update service worker to handle seeking better for videos #60029
Conversation
|
@alxhub can you please review when you get a chance? Thank you. |
|
@kirjs can I get this reviewed please? |
|
@Beanminchild Can you sign the CLA? Once you do that, this should be ok to merge. Thanks! |
sure thing ill do that right now |
@thePunderWoman okay i updated the cla |
…or videos This update ensures that the service worker can handle range requests, allowing video seeking to work correctly when videos are delivered by the service worker.
|
@thePunderWoman everything looks good, do you need anything else from me? |
|
This PR was merged into the repository by commit c663277. The changes were merged into the following branches: main, 20.0.x |
|
This was released in v20.0.1 and caused a major regression in our platform, causing videos to stop working completely. We use shaka-player to play DASH videos hosted in Firebase Cloud Storage. Shaka-player started showing this warning: Payload length does not match range requested bytes,
Reverting back to @angular/service-worker |
|
@anisabboud Please file an issue with a reproduction so we can verify and we can proceed from there. |
|
I think there should be tests in place for this tricky piece of code... angular/packages/service-worker/worker/src/driver.ts Lines 313 to 328 in 4ac0147 This is the shaka-player console error with some additional information (playing DASH video stream encoded by Google Cloud Transcoder and hosted on Firebase Storage): The request range in this example is Perhaps in some cases the Hope this helps. In any case, I appended |
|
BTW in the reference implementation by @philnash (blogpost referenced by @gkalpak), he eventually updated his sw.js code to skip slicing it if In the other reference implementation by the Chrome workbox-range-requests, they also return the response as is if the status is if (originalResponse.status === 206) {
// If we already have a 206, then just pass it through as-is;
// see https://github.com/GoogleChrome/workbox/issues/1720
return originalResponse;
}See this commit: GoogleChrome/workbox@cd5ef34 by @jeffposnick "Return a HTTP 206 response as-is" (to avoid "double-slice"). Perhaps someone from the Chrome/Workbox team could review this PR? |
|
@anisabboud The Angular service worker does not use workbox. I think we'll just revert this pr for now. |
…better for videos (angular#60029)" This reverts commit c663277.
|
@Beanminchild We've reverted this PR in #62422 due to reported regressions. |
@thePunderWoman Thank you for the attention to this issue. I referenced Workbox since they implemented a similar feature, as mentioned in #25865 (comment), which could be used as a basis for this PR / code review. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
"If you ng run app:build:production and your pwa comes with a
Issue Number: #25865
What is the new behavior?
Added logic to driver.ts in the worker src folder that does the following:
The
onFetchmethod now:Checks for Range headers in incoming requests.
Calls the
handleRangeRequestfunction if a Range header is present.The newly added
handleRangeRequestfunction:Parses the Range header to determine the requested byte range.
Fetches the full content and extracts the requested byte range.
Constructs a response with the appropriate Content-Range and Content-Length headers.
Returns a 206 Partial Content response with the requested byte range.
These changes ensure that the service worker can handle Range requests, allowing videos to start at the specified
currentTimewhen delivered by the service worker as well as fixing seeking.Does this PR introduce a breaking change?
Should not be a breaking change