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

Stream video with GridFSBucketAdapter (implements byte-range requests) #6028

Merged
merged 2 commits into from
Sep 11, 2019

Conversation

dplewis
Copy link
Member

@dplewis dplewis commented Sep 5, 2019

Closes: #5834

Similar to #2437

I ran into this issue while trying to view a mov file in safari from the dashboard.

Screen Shot 2019-09-05 at 12 36 31 PM

Safari (iOS) sends a range header for streaming. Range: bytes=704512-709763

This fix creates a stream and gets the data from the start and end range and send the following response.

Screen Shot 2019-09-05 at 4 19 31 PM

Closes: #5834

Similar to #2437

I ran into this issue while trying to view a mov file in safari from the dashboard.
@codecov
Copy link

codecov bot commented Sep 5, 2019

Codecov Report

Merging #6028 into master will decrease coverage by 0.13%.
The diff coverage is 5.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6028      +/-   ##
==========================================
- Coverage   93.91%   93.77%   -0.14%     
==========================================
  Files         164      164              
  Lines       11084    11126      +42     
==========================================
+ Hits        10409    10433      +24     
- Misses        675      693      +18
Impacted Files Coverage Δ
src/Routers/FilesRouter.js 92.98% <ø> (+34.09%) ⬆️
src/Adapters/Files/GridStoreAdapter.js 45.58% <0%> (-40.53%) ⬇️
src/Controllers/FilesController.js 92.5% <0%> (ø) ⬆️
src/Adapters/Files/GridFSBucketAdapter.js 68.33% <15%> (-29.35%) ⬇️
src/triggers.js 94.06% <0%> (-0.58%) ⬇️
src/RestWrite.js 93.56% <0%> (-0.17%) ⬇️
...dapters/Storage/Postgres/PostgresStorageAdapter.js 97.01% <0%> (-0.09%) ⬇️
src/GraphQL/loaders/functionsMutations.js 100% <0%> (ø) ⬆️
src/GraphQL/ParseGraphQLServer.js 93.02% <0%> (ø) ⬆️
src/GraphQL/ParseGraphQLSchema.js 97.02% <0%> (+0.2%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac40bf6...24bf405. Read the comment docs.

@dplewis dplewis requested a review from davimacedo September 5, 2019 23:04
dplewis added a commit to parse-community/parse-server-s3-adapter that referenced this pull request Sep 6, 2019
Copy link
Member

@davimacedo davimacedo left a comment

Choose a reason for hiding this comment

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

It looks good to me! Do you see anyway to add a test case?

@dplewis
Copy link
Member Author

dplewis commented Sep 8, 2019

I can try to add a test. Should this return a promise? I’m planning to add this feature to the AWS S3 adapter

@davimacedo
Copy link
Member

Do you mean when using directAccess: false? When using directAccess: true, I guess that S3 handle this for us.

@dplewis
Copy link
Member Author

dplewis commented Sep 8, 2019

Oh no direct access doesn’t have anything to do with a range request, I created a fix for S3 https://github.com/parse-community/parse-server-s3-adapter/compare/byte-range?expand=1

@davimacedo
Copy link
Member

Nice! It seems good to me as well. I will have a try!

@dplewis
Copy link
Member Author

dplewis commented Sep 8, 2019

Every adapter will implement this differently. The current implementation doesn’t allow for adaptability. Moving handleFileStream will introduce a breaking change since it’s only compatible with GridStoreAdapter but this will force updating the adapters to support this.

@davimacedo
Copy link
Member

davimacedo commented Sep 8, 2019

Got it. What do you think about adding a test case here? Files adapters are supposed to pass these tesss.

@dplewis
Copy link
Member Author

dplewis commented Sep 8, 2019

There currently isn’t a test. A lot of trial and error lead to this fix. I don’t think a test prove this works but I’ll try.

@davimacedo
Copy link
Member

Yes... I imagined that it would be hard. Feel free to merge it with no additional tests if you want.

@dplewis dplewis merged commit 63cabb8 into master Sep 11, 2019
@dplewis dplewis deleted the gridfsbucket-stream branch September 11, 2019 14:34
dplewis added a commit to parse-community/parse-server-s3-adapter that referenced this pull request Sep 11, 2019
* Support Byte Range Requests

See parse-community/parse-server#6028

* use promises

* Add Tests

* rename getFileStream to handleFileStream
UnderratedDev pushed a commit to UnderratedDev/parse-server that referenced this pull request Mar 21, 2020
parse-community#6028)

* Stream video with GridFSBucketAdapter (implements byte-range requests)

Closes: parse-community#5834

Similar to parse-community#2437

I ran into this issue while trying to view a mov file in safari from the dashboard.

* Rename getFileStream to handleFileStream
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.

hope GridFSBucketAdapter support getFileStream interface for client request range content
2 participants