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

Remove 4GB file size workaround for 32bit OS / Stream Videos on IOS #184

Merged
merged 2 commits into from
Jul 9, 2022
Merged

Remove 4GB file size workaround for 32bit OS / Stream Videos on IOS #184

merged 2 commits into from
Jul 9, 2022

Conversation

schoetju
Copy link
Contributor

@schoetju schoetju commented Jul 6, 2022

Like described in #108 there are still issues, when

  • Nextcloud is running on 32 Bit-OS (like mine running on RPI)
  • iOS-Clients want to access Movies

The workaround initially introduced with #74 is not necessary anymore. By the time the 64-Bit-Coding has been improved to serve 4MB chunks that don't hurt on 32Bit-systems.

There are similar recommendations (like https://help.nextcloud.com/t/ios-nextcloud-client-does-not-play-videos/99790/4 ) to step into the 64Bit-Coding always by changing >4 to >0

This Pull-Request simply deletes the separation of 64/32 Bitsystems

@codecov
Copy link

codecov bot commented Jul 8, 2022

Codecov Report

Merging #184 (110e1d0) into master (d7f0d93) will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #184      +/-   ##
============================================
+ Coverage     94.02%   94.11%   +0.09%     
+ Complexity      262      260       -2     
============================================
  Files            15       15              
  Lines           887      884       -3     
============================================
- Hits            834      832       -2     
+ Misses           53       52       -1     
Impacted Files Coverage Δ
lib/Sapi.php 97.19% <100.00%> (+0.83%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

This looks to be a reasonable thing to me. The 4MB "chunking loop" done in the 64-bit case is also a solution for the previous 32-bit problem.

lib/Sapi.php Outdated Show resolved Hide resolved
@phil-davis phil-davis self-requested a review July 8, 2022 16:05
@schoetju
Copy link
Contributor Author

schoetju commented Jul 8, 2022

I've reduced the indent (updated the patch-1 version). code-style checks should be ok now.

Copy link
Member

@staabm staabm left a comment

Choose a reason for hiding this comment

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

Lgtm. Is this code covered by tests?

@schoetju
Copy link
Contributor Author

schoetju commented Jul 8, 2022

It's running fine on my instance (and solved the iOS-Movie issue).

@phil-davis
Copy link
Contributor

codecov is being troublesome because actually the relevant unit tests in tests/HTTP/SapiTest.php are being skipped because they get skipped if xdebug functions are missing. And since changing to GitHub workflows, the CI has been using pcov for coverage, and that stops xdebug from being available.

I am getting that sorted out in PR #185 - then we will get all the unit tests run, and should get better coverage reports.

The workaround is not necessary anymore. By the time the 64-Bit-Coding has been improved to serve 4MB chunks (that don't hurt on 32Bit-systems).
However - the 32Bit Coding still is causing issues on IOS like described in #108

There are similar recommendations (like https://help.nextcloud.com/t/ios-nextcloud-client-does-not-play-videos/99790/4 ) to step into the 64Bit-Coding _always_ by changing >4 to >0
reduced the indent of the block
@phil-davis
Copy link
Contributor

I rebased to get tests running in CI as per #185 - all tests now run and pass, and coverage does not complain.
The code here is covered for cases of small size streamed bodies. There are not specific tests for big files, but for the 64-bit case the code is the same as previous. For 32-bit, we don't have test pipelines that run in a 32-bit environment anyway.

@schoetju thanks - merging

@MichaIng
Copy link

MichaIng commented Oct 21, 2022

It seems the workaround is still required to work with 32-bit systems in Nextcloud: nextcloud/server#34674

I didn't exactly track the issue down to this part of the code, but down to sabre, and the issue appeared with the release which contained this commit the first time.

Since all clients' file sync (+ files app in web UI) are broken on 32-bit systems, I guess video streaming from iOS (which was the reason for removing this workaround) isn't working anymore either.

Not sure where to fix this best, whether to revert this commit, apply a fix for #108 on Nextcloud end, or the other way around, e.g. checking why this 4 MiB chunking theory does not apply, but wanted to bring this to your attention.

@MichaIng
Copy link

I was wrong: The same sabre/http version has been merged into earlier Nextcloud 24 version as well, not causing issues there. It matched so perfectly 😅.

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.

4 participants