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.

Sign up for GitHub

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

Jump to bottom

Use HTTP range requests (responses) to serve mp4 from assets #8219

Closed
wants to merge 2 commits into from
Closed

Use HTTP range requests (responses) to serve mp4 from assets #8219

wants to merge 2 commits into from

Conversation

mroswald
Copy link
Contributor

@mroswald mroswald commented Jun 18, 2016 •
edited
Loading

This PR solves a problem when video assets are used from third-party React Native components (e.g. react-native-video. The video will not work while the assets are served from the react native packager because the used video component (iOS) relies on HTTP range requests.

I added a small fix that allows ranged requests (e.g. mp4) to be served in ranges.

To test this:

  1. make new react native project
  2. add react-native-video to xcode project
  3. add video component to your project
import Video from 'react-native-video';
var resolveAssetSource = require('react-native/Libraries/Image/resolveAssetSource');
/* ... /*
render() {
    let source = resolveAssetSource(require('./someVideoFile.mp4')) || {};
    return <Video /*....*/ source={source} />;
}

That should not work (if video is smaller than a few megabytes, open app a few times). Then add my fix, that should do the trick.

@ghost
Copy link

ghost commented Jun 18, 2016

By analyzing the blame information on this pull request, we identified @frantic and @martinbigio to be potential reviewers.

@ghost ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jun 18, 2016
@mroswald
Copy link
Contributor Author

relates to TheWidlarzGroup/react-native-video#246

@mroswald mroswald force-pushed the feat/stream-mp4-in-ranges branch 2 times, most recently from 4c64464 to 9a4bb8c Compare June 18, 2016 22:44
@mroswald
Copy link
Contributor Author

updated to current master

@mroswald
Copy link
Contributor Author

mroswald commented Jul 5, 2016

rebased to current master

frantic
frantic reviewed Jul 5, 2016
View reviewed changes
packager/react-packager/src/Server/index.js
@@ -168,6 +169,8 @@ const dependencyOpts = declareOpts({
},
});

const isRangeRequest = (req) => req.headers && req.headers.range;
Copy link
Contributor

Choose a reason for hiding this comment

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

why not inline it?

@frantic frantic self-assigned this Jul 5, 2016
@frantic
Copy link
Contributor

frantic commented Jul 5, 2016

Great stuff, thank you!

I have a question and a request:

  1. Is there already an npm package that solves this? Maybe it's easier to add "serves-content-with-ranges" middleware on higher level? This way anything that packager returns could be requested by range.
  2. Mind writing a small unit test for your feature to make sure it doesn't break in the future?

@mroswald
Copy link
Contributor Author

mroswald commented Jul 6, 2016 •
edited
Loading

@frantic thanks for your valuable feedback! I checked some npm packages but didn't find anything that is small enough to fit into React Native's Server module. By the way - you know the reason why they did not simply use express?

anyway: I added a test and refactored the range serving to a small function which I named middleware :-) This is just a proposal, maybe you have a better idea

@frantic
Copy link
Contributor

frantic commented Jul 8, 2016

Looks good, thank you!

@frantic
Copy link
Contributor

frantic commented Jul 8, 2016

@facebook-github-bot import

1 similar comment
@frantic
Copy link
Contributor

frantic commented Jul 11, 2016

@facebook-github-bot import

@ghost
Copy link

ghost commented Jul 11, 2016

Thanks for importing. If you are an FB employee go to Phabricator to review.

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 12, 2016
@ghost ghost closed this in 91ff686 Jul 13, 2016
@nikki93 nikki93 mentioned this pull request Aug 6, 2016
Closed
ghost pushed a commit that referenced this pull request Aug 6, 2016
@nikki93
Fix off-by-one error in range requests
a241706 
Summary:
#8219 adds range requests to the asset server, but there was an off-by-one-error that made responses end prematurely. This made (for example) react-native-video not work for video assets. This change fixes the off-by-one error and react-native-video works with assets.

**Test plan (required)**

Try the test in the original pull request for range requests: #8219
Closes #9254

Differential Revision: D3680070

fbshipit-source-id: 3f2a18ba9f35b45b340f4a1046bc099b8444eb7d
nikki93 added a commit to expo/react-native that referenced this pull request Aug 6, 2016
@nikki93
Fix off-by-one error in range requests
d102590 
Summary:
facebook#8219 adds range requests to the asset server, but there was an off-by-one-error that made responses end prematurely. This made (for example) react-native-video not work for video assets. This change fixes the off-by-one error and react-native-video works with assets.

**Test plan (required)**

Try the test in the original pull request for range requests: facebook#8219
Closes facebook#9254

Differential Revision: D3680070

fbshipit-source-id: 3f2a18ba9f35b45b340f4a1046bc099b8444eb7d
cmcewen pushed a commit to cmcewen/react-native that referenced this pull request Aug 15, 2016
@nikki93 @cmcewen
Fix off-by-one error in range requests
107546f 
Summary:
facebook#8219 adds range requests to the asset server, but there was an off-by-one-error that made responses end prematurely. This made (for example) react-native-video not work for video assets. This change fixes the off-by-one error and react-native-video works with assets.

**Test plan (required)**

Try the test in the original pull request for range requests: facebook#8219
Closes facebook#9254

Differential Revision: D3680070

fbshipit-source-id: 3f2a18ba9f35b45b340f4a1046bc099b8444eb7d
samerce pushed a commit to iodine/react-native that referenced this pull request Aug 23, 2016
@mroswald @samerce
Use HTTP range requests (responses) to serve mp4 from assets
f3e3cff 
Summary:
This PR solves a problem when video assets are used from third-party React Native components (e.g. [react-native-video](https://github.com/brentvatne/react-native-video). The video will not work while the assets are served from the react native packager because the used video component (iOS) relies on HTTP range requests.

I added a small fix that allows ranged requests (e.g. mp4) to be served in ranges.

To test this:

1. make new react native project
1. add [react-native-video](https://github.com/brentvatne/react-native-video) to xcode project
1. add video component to your project
```
import Video from 'react-native-video';
var resolveAssetSource = require('react-native/Libraries/Image/resolveAssetSource');
/* ... /*
render() {
    let source = resolveAssetSource(require('./someVideoFile.mp4')) || {};
    return <Video /*....*/ source={source} />;
}
```

That should not work (if video is smaller than a few megabytes, open app a few times). Then add my fix, that should do the trick.
Closes facebook#8219

Reviewed By: davidaurelio

Differential Revision: D3542485

Pulled By: frantic

fbshipit-source-id: e4f2e4d3aaafa8445e965259bf04ad107dba8a4f
mpretty-cyro pushed a commit to HomePass/react-native that referenced this pull request Aug 25, 2016
@mroswald
Use HTTP range requests (responses) to serve mp4 from assets
bc5be58 
Summary:
This PR solves a problem when video assets are used from third-party React Native components (e.g. [react-native-video](https://github.com/brentvatne/react-native-video). The video will not work while the assets are served from the react native packager because the used video component (iOS) relies on HTTP range requests.

I added a small fix that allows ranged requests (e.g. mp4) to be served in ranges.

To test this:

1. make new react native project
1. add [react-native-video](https://github.com/brentvatne/react-native-video) to xcode project
1. add video component to your project
```
import Video from 'react-native-video';
var resolveAssetSource = require('react-native/Libraries/Image/resolveAssetSource');
/* ... /*
render() {
    let source = resolveAssetSource(require('./someVideoFile.mp4')) || {};
    return <Video /*....*/ source={source} />;
}
```

That should not work (if video is smaller than a few megabytes, open app a few times). Then add my fix, that should do the trick.
Closes facebook#8219

Reviewed By: davidaurelio

Differential Revision: D3542485

Pulled By: frantic

fbshipit-source-id: e4f2e4d3aaafa8445e965259bf04ad107dba8a4f
mpretty-cyro pushed a commit to HomePass/react-native that referenced this pull request Aug 25, 2016
@nikki93
Fix off-by-one error in range requests
1ea8fa4 
Summary:
facebook#8219 adds range requests to the asset server, but there was an off-by-one-error that made responses end prematurely. This made (for example) react-native-video not work for video assets. This change fixes the off-by-one error and react-native-video works with assets.

**Test plan (required)**

Try the test in the original pull request for range requests: facebook#8219
Closes facebook#9254

Differential Revision: D3680070

fbshipit-source-id: 3f2a18ba9f35b45b340f4a1046bc099b8444eb7d
cpojer pushed a commit to facebook/metro that referenced this pull request Jan 26, 2017
@mroswald
Use HTTP range requests (responses) to serve mp4 from assets
9d64142 
Summary:
This PR solves a problem when video assets are used from third-party React Native components (e.g. [react-native-video](https://github.com/brentvatne/react-native-video). The video will not work while the assets are served from the react native packager because the used video component (iOS) relies on HTTP range requests.

I added a small fix that allows ranged requests (e.g. mp4) to be served in ranges.

To test this:

1. make new react native project
1. add [react-native-video](https://github.com/brentvatne/react-native-video) to xcode project
1. add video component to your project
```
import Video from 'react-native-video';
var resolveAssetSource = require('react-native/Libraries/Image/resolveAssetSource');
/* ... /*
render() {
    let source = resolveAssetSource(require('./someVideoFile.mp4')) || {};
    return <Video /*....*/ source={source} />;
}
```

That should not work (if video is smaller than a few megabytes, open app a few times). Then add my fix, that should do the trick.
Closes facebook/react-native#8219

Reviewed By: davidaurelio

Differential Revision: D3542485

Pulled By: frantic

fbshipit-source-id: e4f2e4d3aaafa8445e965259bf04ad107dba8a4f
cpojer pushed a commit to facebook/metro that referenced this pull request Jan 26, 2017
@nikki93
Fix off-by-one error in range requests
0b9ca22 
Summary:
facebook/react-native#8219 adds range requests to the asset server, but there was an off-by-one-error that made responses end prematurely. This made (for example) react-native-video not work for video assets. This change fixes the off-by-one error and react-native-video works with assets.

**Test plan (required)**

Try the test in the original pull request for range requests: facebook/react-native#8219
Closes facebook/react-native#9254

Differential Revision: D3680070

fbshipit-source-id: 3f2a18ba9f35b45b340f4a1046bc099b8444eb7d
tungdo194 pushed a commit to tungdo194/rn-test that referenced this pull request Apr 28, 2024
@mroswald @seraj-ahmad
Use HTTP range requests (responses) to serve mp4 from assets
0744ffb 
Summary:
This PR solves a problem when video assets are used from third-party React Native components (e.g. [react-native-video](https://github.com/brentvatne/react-native-video). The video will not work while the assets are served from the react native packager because the used video component (iOS) relies on HTTP range requests.

I added a small fix that allows ranged requests (e.g. mp4) to be served in ranges.

To test this:

1. make new react native project
1. add [react-native-video](https://github.com/brentvatne/react-native-video) to xcode project
1. add video component to your project
```
import Video from 'react-native-video';
var resolveAssetSource = require('react-native/Libraries/Image/resolveAssetSource');
/* ... /*
render() {
    let source = resolveAssetSource(require('./someVideoFile.mp4')) || {};
    return <Video /*....*/ source={source} />;
}
```

That should not work (if video is smaller than a few megabytes, open app a few times). Then add my fix, that should do the trick.
Closes facebook/react-native#8219

Reviewed By: davidaurelio

Differential Revision: D3542485

Pulled By: frantic

fbshipit-source-id: e4f2e4d3aaafa8445e965259bf04ad107dba8a4f
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@frantic frantic

Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mroswald @frantic