Allow video skip with Accept-Ranges closes #457 #462#466
Conversation
| ? res.locals.path | ||
| : req.path | ||
| ldp.exists(req.hostname, reqPath, (err, stat) => { | ||
| ldp.exists(req.hostname, reqPath, (err, ret) => { |
There was a problem hiding this comment.
What does ret stand for?
|
@dmitrizagidulin ret == return value as an object Reason is that get has to pass back a number of items, such as the range, the stream, etc. Changing it into an object gives more flexibility. |
|
Looks sensible to me |
timbl
left a comment
timbl
left a comment
There was a problem hiding this comment.
Suggest another review with someone who knows the code better.
|
Ok, I have run it on a test machine , seems to work nicely! Video can be skipped backward amd forward and started at any point. |
nicola
left a comment
nicola
left a comment
There was a problem hiding this comment.
Variables shouldn't be declared in a block scope, also, these changes need testing, why did this got merged?
| ldp.exists(req.hostname, reqPath, (err, stat) => { | ||
| ldp.exists(req.hostname, reqPath, (err, ret) => { | ||
| if (ret) { | ||
| var stat = ret.stream |
There was a problem hiding this comment.
Wait this doesn't sound right
| var contentType = ret.contentType | ||
| var container = ret.container | ||
| var contentRange = ret.contentRange | ||
| var chunksize = ret.chunksize |
There was a problem hiding this comment.
Why do you create variables inside a block scope?
| } | ||
|
|
||
| if (ret) { | ||
| var stats = ret.stream |
| return callback(notexists) | ||
| } | ||
| if (ret) { | ||
| var stream = ret.stream |
| var start = parseInt(partialstart, 10) | ||
| var end = partialend ? parseInt(partialend, 10) : total - 1 | ||
| var chunksize = (end - start) + 1 | ||
| var contentRange = 'bytes ' + start + '-' + end + '/' + total |
|
|
||
| get (host, reqPath, baseUri, includeBody, contentType, callback) { | ||
| get (options, callback) { | ||
| if (options) { |
There was a problem hiding this comment.
What if no options are passed?
|
Hi @nicola thanks for the review! I asked about how to do this on gitter and in the issues since September. Since I didnt get any feedback I tried to make a patch based on my understanding of the code and what I have learnt in nodejs. I'd be happy to refactor based on best practice recommendations. Regarding variables in block scope, is that against our coding standards? My normal coding principle is to declare variables close to where they are used. As I understand it they are hoisted to the top of the function in JS. I could do it another way. If no options are passed it is the same as sending in undefined in the positional parameters, I think? Re: Advice welcome! |
|
Hey @melvincarvalho of course your PR is super welcome, it really great to make a PR instead of just discussing changes! (and you even gave a test passing working PR, which is amazing!) However, I am not sure it was ready to merge yet. The only parts that I don't find ready are the declarations of variables in a scope, they should be declared in the same scope as the others, which are mainly all the changes that I am pointing out. Let me know if that makes sense or we can find some time to do it together |
|
@nicola thanks again for the kind feedback! :) What I understand of JavaScript is that there is not block scoped variables with the keyword When returning from There is some inconsistency as sometimes the callback returns the stat, and sometimes the stream, and different calling functions use it differently. That's why the names look a bit strange, but that could be improved with a refactor. However, this was the original logic of the current code. We can discuss this further here or in gitter, if that suits you. |
4 participants
This PR allows video to be skipped using byte ranges.
It also refactors ldp.get to use options rather than positional variables in order to provide more flexibility.
Separation of concerns between ldp.js and handers/get.js is maintained