Skip to content

document enhance, and removed depreciate package#221

Merged
dr-dimitru merged 4 commits intoveliovgroup:devfrom
salmanhasni:gd-enhancement
Oct 12, 2016
Merged

document enhance, and removed depreciate package#221
dr-dimitru merged 4 commits intoveliovgroup:devfrom
salmanhasni:gd-enhancement

Conversation

@salmanhasni
Copy link
Copy Markdown
Contributor

@salmanhasni salmanhasni commented Sep 5, 2016

ISSUES:
1-gcloud-node official pacakge is now rename to google-cloud-node
2-No need to make file public (get file with bucket authentication and pipe it to meteor application user)

@salmanhasni
Copy link
Copy Markdown
Contributor Author

@dr-dimitru Can you please review this

headers: _.pick(http.request.headers, 'range', 'accept-language', 'accept', 'cache-control', 'pragma', 'connection', 'upgrade-insecure-requests', 'user-agent')
}).pipe(http.response);
var remoteReadStream = bucket.file(path).createReadStream();
remoteReadStream.pipe(http.response);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@salmanhasni
Have you tested this with video/audio files, is time seeking working?

Copy link
Copy Markdown
Contributor Author

@salmanhasni salmanhasni Sep 6, 2016

Choose a reason for hiding this comment

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

@dr-dimitru
tested it with static files (images, docs)
need to test it with audio/video files for time seeking.

I will let you know once i will test it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@salmanhasni waiting for test results

So you need test results for:

  • Standart request for static and streaming files (seeking may or may not work)
  • Download (with ?download=true query) for static and streaming files (browser should show remaining time and speed)
  • Streaming (with ?play=true query) for static and streaming files (time seeking must work)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@salmanhasni any news on your end?

Copy link
Copy Markdown
Contributor Author

@salmanhasni salmanhasni Sep 16, 2016

Choose a reason for hiding this comment

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

@dr-dimitru sorry for the delay response, i was out for vacations.
I have tested static (images etc) and dynamic files ( video, audio),
standard and download request works fine, where as time seeking having issue in streaming files ( with ?play=truequery).

Copy link
Copy Markdown
Contributor Author

@salmanhasni salmanhasni Sep 18, 2016

Choose a reason for hiding this comment

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

@dr-dimitru I am able to make this PR work, but for that I need to write the below redundant code (logic already exist in serve() method but only for readableStream == null) for making stream videos work with time seeking.
Should i refactor it ?

    if http.request.headers.range
      partiral = true
      array    = http.request.headers.range.split /bytes=([0-9]*)-([0-9]*)/
      start    = parseInt array[1]
      end      = parseInt array[2]
      end      = vRef.size - 1 if isNaN(end)
      take     = end - start
    else
      start    = 0
      end      = vRef.size - 1
      take     = vRef.size

    if partiral or (http.params.query.play and http.params.query.play == 'true')
      reqRange = {start, end}
      if isNaN(start) and not isNaN end
        reqRange.start = end - take
        reqRange.end   = end
      if not isNaN(start) and isNaN end
        reqRange.start = start
        reqRange.end   = start + take

      reqRange.end = vRef.size - 1 if ((start + take) >= vRef.size)

      if self.strict and (reqRange.start >= (vRef.size - 1) or reqRange.end > (vRef.size - 1))
        responseType = '416'
      else
        responseType = '206'
    else
      responseType = '200'

    var remoteReadStream;

    if responseType == "206"
      remoteReadStream = bucket.file(path).createReadStream({start: reqRange.start, end: reqRange.end});
    else if responseType == "200"
      remoteReadStream = bucket.file(path).createReadStream();

    self.serve(http, fileRef, fileRef.versions[version], version, remoteReadStream);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just pass remoteReadStream as 5th argument to serve() method.
No need write more code, see this example: https://github.com/VeliovGroup/Meteor-Files/blob/master/demo/lib/files.collection.coffee#L86

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@salmanhasni looks like I'm wrong...
Is there a way to .createReadStream form bucket with start and end?

Copy link
Copy Markdown
Contributor Author

@salmanhasni salmanhasni Sep 18, 2016

Choose a reason for hiding this comment

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

@dr-dimitru Yes we can.

Sorry for my side, I just forgot to pass reqRange parameter in createReadStream() method call for responseType "206", please see the above update code.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@salmanhasni
Sounds like you made it work, right?
If so, does tests passed?
If tests is passed re-open this PR, and I'll merge it. :)

@salmanhasni
Copy link
Copy Markdown
Contributor Author

@dr-dimitru
All test passed,
we are good to proceed.

@dr-dimitru
Copy link
Copy Markdown
Member

@salmanhasni great. Thank you for this.
I'll do test on my end, and merge this PR.

@salmanhasni
Copy link
Copy Markdown
Contributor Author

@dr-dimitru Sure

@dr-dimitru dr-dimitru merged commit cc2c08b into veliovgroup:dev Oct 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants