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

Express server doesn't support content-range for serving local video/media files #8888

Open
danDanV1 opened this issue Oct 15, 2019 · 10 comments

Comments

@danDanV1
Copy link

TLDR;
Cannot seek playback time of a video file served from localhost during Tests because ember-cli's express server configuration doesn't support Content-Range and serves the file with a 200 response instead of 206 partial content.

Details:
This is a known issue for how Chrome handles video content. Chrome makes multiple HTTP request with Range header for media file and expects HTTP Content-Range and Content-Length response header with 206 Partial Content status. If server send '200 OK' status Chrome will not play the media file. If server send '200 OK' status with response header 'Accept-Ranges: none' then Chrome will play media file, but seeking/jumping to a specific time will not work.

Reproduce:
Insert a video into your template and look at the status code. If the video is served locally it will have a 200 response and seeking playback to a specific time will not work, if it is served externally from a CDN it will have a 206 response and seeking playback works.

Recommendations:
A) Instead of an Express server, we use http-server which support partial content.
B) Add support for serving partial-content (see gist below)

Resources:
https://gist.github.com/padenot/1324734 (How to serve media on node.js)
https://medium.com/better-programming/video-stream-with-node-js-and-html5-320b3191a6b6


Output from ember version --verbose && npm --version && yarn --version:

ember-cli: 3.10.1
http_parser: 2.8.0
node: 10.16.0
v8: 6.8.275.32-node.52
uv: 1.28.0
zlib: 1.2.11
brotli: 1.0.7
ares: 1.15.0
modules: 64
nghttp2: 1.34.0
napi: 4
openssl: 1.1.1b
icu: 64.2
unicode: 12.1
cldr: 35.1
tz: 2019a
os: darwin x64
6.9.0
1.17.3

@rwjblue
Copy link
Member

rwjblue commented Oct 16, 2019

Would definitely love help pushing this one forward...

@rwjblue
Copy link
Member

rwjblue commented Oct 16, 2019

I did a little bit of poking around, seems like we need to tweak the logic here:

https://github.com/broccolijs/broccoli/blob/7d61e752fe72ce4eeac73d0c2b053c6aa7e9e8e9/lib/middleware.ts#L130-L147

To be more like:

          lastModified = stat.mtime.toUTCString();
          response.setHeader('Last-Modified', lastModified);
          // nginx style treat last-modified as a tag since browsers echo it back
          if (request.headers['if-modified-since'] === lastModified && !request.headers.range) {
            response.writeHead(304);
            response.end();
            return;
          }

          const mimeType = mime.contentType(path.extname(filename));
          // read file sync so we don't hold open the file creating a race with
          // the builder (Windows does not allow us to delete while the file is open).
          buffer = fs.readFileSync(filename);

          if (request.headers.range) {
            const range = request.headers.range;
            const total = content.length;
            const [partialstart, partialend] = range.replace(/bytes=/, "").split("-");

            const start = parseInt(partialstart, 10);
            const end = partialend ? parseInt(partialend, 10) : total;
            const chunksize = end - start;

            response.setHeader('Content-Range', `bytes ${start}-${end}/${total}`);
            response.setHeader('Accept-Ranges', 'bytes');
            response.setHeader('Content-Length', chunksize);
            response.setHeader('Content-Type', mimeType);
            response.writeHead(206);
            response.end(buffer.slice(start, end));
            return;
          } else {
            response.setHeader('Cache-Control', 'private, max-age=0, must-revalidate');
            response.setHeader('Content-Length', stat.size);
            response.setHeader('Content-Type', mimeType);
            response.writeHead(200);
            response.end(buffer);
          }

@danDanV1
Copy link
Author

Looks good to me @rwjblue. Has all the headers I believe we need, particularly:
eg.

    accept-ranges: bytes
    Content-Length: 2732878
    Content-Range: bytes 0-2732877/2732878
    status: 206 

I just made a series of test to help with this, they compare the responses of video hosted on a CDN (what we should be getting back) vs one in the local assets (failing). It includes a video file for testing too.
https://github.com/edeis53/ember-cli-partialcontent

I did it on both the Unit level, just checking the headers of the response, and also an integration test where a video element is rendered on the page and test whether seeking in Chrome works.

@tavosansal
Copy link

I am trying to play a local video (public folder) on Safari and it is not working. Could this be why?

@nbibler
Copy link

nbibler commented Sep 16, 2020

@tavosansal : Yes, this is likely where your problem is coming from. Safari is more strict about how it handles server responses to Range requests than Chrome. In my experience, if the server incorrectly replies (which Express tends to do by default), Safari will generally treat it as an unreadable media.

@nbibler
Copy link

nbibler commented Sep 16, 2020

The following request/response was recorded using Ember CLI 3.21.2, Safari 13.1.2, and a Video JS player with a "preload=metadata" setting. In it, you can see a Range request going out the door, but Express replying with a standard 200 (not 206) response:

express-safari-issues

@rwjblue
Copy link
Member

rwjblue commented Sep 18, 2020

Would definitely love help pushing this one forward...

😸

@nbibler
Copy link

nbibler commented Sep 18, 2020

Yeah. I went down a rabbit hole with Safari yesterday and found a browser bug. Reported it to Apple with a demo video, but haven't yet (and don't really expect to) heard anything back.

I haven't yet wrapped my head around why Broccoli is managing a middleware for Express. And even when it does, why it's not somehow delegating to Express's serve-static. That middleware already supports all of the behaviors we need. The complication is that it's expecting to point to static directories for the static files, where Broccoli is dealing with dynamically created and changing directories (I assume).

@gabrielcsapo
Copy link

@nbibler we are seeing similar issues with not using serve-static for our application use case. We are investigating how to remove the middleware in broccoli to utilize serve-static.

@gabrielcsapo
Copy link

I wonder if it would make more sense to use serve-static in https://github.com/ember-cli/broccoli-middleware/blob/master/lib/serve-asset-middleware.js and replace the custom serve code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants