-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Stream video with GridStoreAdapter (implements byte-range requests) #2437
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 GridStoreAdapter (implements byte-range requests) #2437
Conversation
Current coverage is 91.76% (diff: 40.81%)@@ master #2437 diff @@
==========================================
Files 93 93
Lines 10602 10718 +116
Methods 1309 1320 +11
Messages 0 0
Branches 1721 1743 +22
==========================================
+ Hits 9804 9835 +31
- Misses 798 883 +85
Partials 0 0
|
|
||
gridFile.seek(start, function () { | ||
// get gridFile stream | ||
var stream = gridFile.stream(true); |
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.
fix indent.
Hi @Bragegs, Thanks for your PR, that seems to be pretty useful! Can you address the different comments? And fix the indents to 2 spaces instead of 4? |
handleVideoStream(filename, range, res, contentType) { | ||
return this._connect().then(database => { | ||
return GridStore.exist(database, filename) | ||
.then(() => { |
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.
please put on the same line as previous one
Hi @flovilmart! Will address the typos Monday when returning from holiday, sry for the inconvenience. "shouldn't we infer the streaming based upon the request headers like Range.": "Also the header seem to be Content-Range not Range.": |
* If not; handle the request as usual with "getFileData". | ||
*/ | ||
handleVideoStream(filename, range, res, contentType) { | ||
if (this.adapter.constructor.name == 'GridStoreAdapter') { |
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.
Can you expose just check if the function exists on the adapter for forward compatibility?
Another major nit, would it be possible to return a promise with the stream (and other things like length etc...) so the adapter don't need to know about setting headers and don't get the express response expose to keep the separation of concerns? |
@Bragegs updated the pull request. |
@Bragegs updated the pull request. |
…with_gridstoreadapter # Conflicts: # src/Routers/FilesRouter.js
@Bragegs updated the pull request. |
res.end('File not found.'); | ||
}); | ||
const contentType = mime.lookup(filename); | ||
if (req.get['Range']) { |
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.
Can we just check that typeof this.adapter.getFileStream === 'function'
This way we'll fall through regular response for non supporting adapters and it will be forward compatible?
@Bragegs updated the pull request. |
@flovilmart Thx for guiding me through! You doin a great job |
Is this still in the latest version? I am using version 2.2.25 and I do not see the code changes mentioned in @Bragegs document https://notehub.org/8ncql |
This should be available since a while. |
@flovilmart I am looking at the changes to the files mentioned int he above link form @Bragegs and I do not see them in my version, nor do I see them in the current version of this repo. For example, I do a ctrl-f for "video" in parse-server-example\node_modules\parse-server\lib\Routers\FilesRouter and I only get 1 hit from a comment, there should be for example a 'var video' according to the link https://notehub.org/8ncql Update: |
@flovilmart any updates on my last post? |
If you made changes directly in the lib folder, this won't be deployed as npm will always fetch the build package from npm. |
@flovilmart Maybe you did not understand what I am saying. For starters let me say that I am using parse-server-example. What I did is I made the code changes as described above in the parse-server folder of parse-server-example's node_modules folder. When deployed to AWS everything works fine except video streaming, I get an error regarding the code changes I made "handleVideoStream is not a function". So technically, I believe you did not understand my question because clearly the code update is not affected when pm runs because it sees that I already have the latest version so it will not update (probably nam looks at some file somewhere to verify that). Look, I beg for you to really look into this. NONE of the code changes described here show up in the files...maybe someone removed them in future versions of parse-server. Furthermore, try building the parse-server-example project and look at the parse-server files int he node_modules after you run "nam install", you will notice that there is no "src" folder, just a "lib" folder. Now I may be a noob so forgive me if my understanding is bad, but why is it that when you clone parse-server by itself, all the files get sync as is seen int he github, but within parse-server-example it is not? |
The src folder is built into the lib folder through babel upon publishing the module. The lib folder is not committed to git as it would be inconvenient.
There is no reference of a 'var video' whatsoever in that pull request, I'm not sure what you're referring to. Any changes made to parse-server should be done in the src folder, run through If you wish to run your own fork of parse-server with your changes, you need to change multiple things. @acinader had an example at one point how to achieve that. |
As I said in my first comment, I am referencing this https://notehub.org/8ncql when I speak about code changes, these changes seem to have fixed most people's issues with the video streaming. Either way, I cannot for the life of me figure out why I still cannot stream a video that I posted. It works through web when I download my videos in Chrome but not in mobile iOS or Safari, which makes sense given the comment above "HTTP servers hosting media files for iOS must support byte-range requests, which iOS uses to perform random access in media playback. (Byte-range support is also known as content-range or partial-range support.) Most, but not all, HTTP 1.1 servers already support byte-range requests." I am looking at the pull requests and I see that some of the code is not in the newest version |
What do you mean by that???? The code is here... You're looking to a particular commit, where the naming was |
Ok, thanks for the clarification. So everything is ok with the code then. But the problem persists. I cannot stream videos. I have deployed to parse-server-example with AWS Elastic Bean Stalk, backed with a t2.small ec2 instance. I have updated nginx so that I can upload files of size 20mb, so I can successfully post videos. I can;t stream videos and there are no errors in my logs, so I see all the GET requests coming in for mp4 files and no issues are reported after the request |
@Bragegs may be more likely to answer that problem, I'm not sure if it's related to the headers not sent correctly, iOS behaving weirdly, or anything else. Feel free to open an issue with filling the issue template completely with all relevant logs, server AND client. |
@flovilmart Thank you. One last question. Since with the old parse server (pre-open source) when parse was not shut down everything was working fine, some of the issues we are having now must be related to the way in which people are deploying their instances correct? For example, if I were to run my parse-server with an https server I do not think I would be getting this issue with the video streaming. Also, I remember reading that old parse-server (pre-open source) was using AWS S3 to store content, is that true? Because if it is, it would suggest that i may be able to fix the issue doing that (which I will test anyways). This would also validate @dpaid comment on this thread where he/she claims that the S3 adapter fixed the streaming issue (even before @Bragegs fix). Am I thinking along the right lines here? |
@Miladinho if you're deploying on elastic beanstalk, do you have an Elastic Load Balancer in front of your instances? Do you know if they are classic or app? If they are classic, I don't think that you are going to be able to stream using ws protocol.....sorry if i am way of topic :) |
@acinader Hi, Thank you for contributing your help. I actually just added the load balancer so I guess it is now an app instance. But it is not working. I added it because I thought of two solutions, 1 was to run HTTPS for which I needed to add the load balancer, 2 was to try saving files to S3. Right now it is running S# with load balancer and I still can't get the stream to work. |
@Miladinho well, there are two different types of load balancers. classic and application. how do you know that you configured an application load balancer? for me, i am doing all of the config from the command line, so I don't know how to tell you to check using the console. here's the docs on how to configure the application load balancer. FYI, I haven't set up web streaming yet, but will need to pretty soon. until then, my ability to help you is very limited..... |
Hi @acinader |
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
Apple :
"HTTP servers hosting media files for iOS must support byte-range requests, which iOS uses to perform random access in media playback. (Byte-range support is also known as content-range or partial-range support.) Most, but not all, HTTP 1.1 servers already support byte-range requests."
Allows users of GridStoreAdapter to stream videos to IOS devices. Not tested on Android device.
(First PR)