-
-
Notifications
You must be signed in to change notification settings - Fork 70
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
Support range requests for video content #768
Conversation
@BPerlakiH Codefactor should pass and I also see a few style problems. |
….com:kiwix/apple into 767-support-range-requests-for-video-content
Please see it updated. |
/// As a result of the large volume of small requests, CPU usage will be very high, | ||
/// which can result in app or webpage frozen. | ||
/// To mitigate, opting for the less "broken" behavior of ignoring Range header | ||
/// until WebKit behavior is changed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did found the root cause of this behaviour. In the original PR the byte range was one off, eg:
Byte-Range: 0-5
Content-Length: 5
Actually it should be either:
Byte-Range: 0-4
Content-Length: 5
OR
Byte-Range: 0-5
Content-Length: 6
Due to this one off error the browser was "kept" downloading the end of the range in chunks about ~8 bytes long.
Once the range was corrected, this side effect no longer exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure about this? I am still seeing the 8b range requests on this branch, with mp4 videos. webm videos did not trigger those small range requests…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well... I did tested with mp4 files, and that is indeed triggering 8 bytes.
I also noticed that the "videojs" version is different on the ZIM file with mp4, than on the ZIM file with the webm test videos.
Maybe we will need an identical JS setup for both webm and mp4.
self?.sendHTTP404Response(urlSchemeTask, url: url) | ||
self?.stopFor(hash) | ||
return | ||
} | ||
self?.sendHTTP200Response(urlSchemeTask, url: url, content: content) | ||
if isVideo { | ||
self?.sendHTTP206Response(urlSchemeTask, url: url, content: content, start: start, end: end) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we only apply the range responses to video type of contents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only apply to but also Apply to ALL videos…
@@ -161,6 +177,11 @@ struct URLContent { | |||
return mime | |||
} | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This currently uses the ZIM file's modification date time to create the eTag, which I think a good enough solution for our purposes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is our purpose? Is this related to Range-requests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking here was, that I looked at what headers are returned in Safari / Caddy when webm is played.
So I wanted to have the very same ones, to make sure it works. One of them was this eTag, which can be generated based on the date the ZIM file was changed.
We can omit this if you think it is not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No harm in keeping it IMO. ETag is supposed to be a uniq version ID for a resource and this implementation is OK in this regard since all resources are single-version made at the same time in a ZIM.
Still it feels like abusing the header to have the same for all resources 😀
Mandates a comment explaining this I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about the long delay !! 😵💫
PR has no description while it introduces changes that seems unrelated to the linked ticket: all that date stuff, is it helping the range requests in anyway? Or is it just general improvements?
Otherwise Code looks OK but it doesn't seem to fix what it intends to: webm video did not trigger the many range request so I'm not looking for improvement there.
MP4 videos on the other hand was triggering the 8b requests… and still is with this branch
![Screenshot 2024-05-23 at 17 52 26](https://private-user-images.githubusercontent.com/57929/333292034-2d4df319-dd4e-46e0-9f9b-1771392b8d35.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU1OTIzMzEsIm5iZiI6MTcyNTU5MjAzMSwicGF0aCI6Ii81NzkyOS8zMzMyOTIwMzQtMmQ0ZGYzMTktZGQ0ZS00NmUwLTlmOWItMTc3MTM5MmI4ZDM1LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MDYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTA2VDAzMDcxMVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTU1MWUyZjMwZmUwYzc0NGQ2YzFkYzg0YjFjMGZhOTVlYzRiMzI0ZjhmOTY3YWQxMWVhOWY2Njc5MWYxNDgwOTkmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.mEqEOogJO0P0zlepnLjz2sGXoWYbQXw29AoEw88_JUo)
You are also returning 206 responses to all videos, regarding of whether a range-request was made. While mostly harmful, this is does not respect HTTP
@@ -161,6 +177,11 @@ struct URLContent { | |||
return mime | |||
} | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is our purpose? Is this related to Range-requests?
|
||
import Foundation | ||
|
||
extension Date { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, related to ranges or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is related to the returned response header, eg. as on your screenshot, we have the Response:
Last-Modified: Thu, 16 May 2024 11:38:20 GMT
in order to have the date of the ZIM file formatted the same way, we need this change.
Again, if you think this is not needed, I can remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it's fine ; just wanted to make sure it was a side thing
/// As a result of the large volume of small requests, CPU usage will be very high, | ||
/// which can result in app or webpage frozen. | ||
/// To mitigate, opting for the less "broken" behavior of ignoring Range header | ||
/// until WebKit behavior is changed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure about this? I am still seeing the 8b range requests on this branch, with mp4 videos. webm videos did not trigger those small range requests…
self?.sendHTTP404Response(urlSchemeTask, url: url) | ||
self?.stopFor(hash) | ||
return | ||
} | ||
self?.sendHTTP200Response(urlSchemeTask, url: url, content: content) | ||
if isVideo { | ||
self?.sendHTTP206Response(urlSchemeTask, url: url, content: content, start: start, end: end) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only apply to but also Apply to ALL videos…
Waiting that #784 is properly implemented to move forward. |
ThisnPR shiukd be rebased and completed has nothing else is blocking. |
Closing in favour of #894 |
Fixes: #767