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

add srcObject for getUserMedia #9146

Closed
wants to merge 3 commits into from

Conversation

acidsound
Copy link

srcObject is useful when use video elements in webRTC video chat.
video.srcObject = stream is standard and well-implemented.
you can use <video src={URL.createObjectURL(stream)}>
but it's a hack and deprecated. don't use it.
<video srcObject={stream}> is better. neat and clean.

srcObject is useful when use `video` elements in webRTC video chat.
`video.srcObject = stream` is standard and well-implemented. 
you can use `video.src=URL.createObjectURL(stream)` or `video.src=stream.toURL()`.
but it's a hack and deprecated. don't use it.
@michael-donat
Copy link

michael-donat commented Jun 29, 2017

Hi, this is a small change that I would love seeing merged. Chrome is throwing an advisory on turning streams into urls for use with audio/video tags.

Copy link
Contributor

@aweary aweary left a comment

Choose a reason for hiding this comment

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

A small change request. Thanks @acidsound!

@@ -147,6 +147,7 @@ var HTMLDOMPropertyConfig = {
srcDoc: 0,
srcLang: 0,
srcSet: 0,
srcObject: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like srcObject should be set as a property on the element, not as an attribute. We should use MUST_USE_PROPERTY instead of 0 here to force React to set it as a property on the element.

Copy link

Choose a reason for hiding this comment

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

I'm not sure I follow here. What is the difference between property and an attribute?

srcObject is no different to src really, the only difference is that one accepts a stream while the other a url.

Copy link
Contributor

Choose a reason for hiding this comment

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

The difference is between:

video.setAttribute('srcObject', stream)

and

video.srcObject = stream

Since stream is likely an instance of MediaStream, setting it as an attribute wouldn't work since it would coerce it to a string

<video srcobject="[object MediaStream]"></video>

Choose a reason for hiding this comment

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

Ahh, makes sense, thanks for explaining this!

Copy link
Contributor

@nhunzaker nhunzaker left a comment

Choose a reason for hiding this comment

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

Approved with @aweary changes.

Just curious: what would happen if you tried to server render this?

acidsound added a commit to acidsound/react that referenced this pull request Jul 31, 2017
@acidsound
Copy link
Author

Okay @aweary . I fixed it 45b3d4d. Plz review again.

@syranide
Copy link
Contributor

syranide commented Aug 2, 2017

AFAIK this goes against previous decisions not to expose "property-only" properties, because they cannot be used in SSR and the scope becomes massively bigger (there are tons of such properties). For all practical purposes just setting the property manually in a life-cycle method accomplishes the same thing, it being handled by ReactDOM directly does not improve the behavior (as opposed to attributes where it can).

@akshaykmr
Copy link

@acidsound Your changes look good. I think you should squash your two commits into one 👍.

@chyzwar
Copy link

chyzwar commented Sep 27, 2017

@syranide Chrome and Firefox and Safari plans to deprecate createObjectURL, react will not support MediaStream in video element?

Should I create a new pull request or this one can be completed?

@syranide
Copy link
Contributor

@chyzwar AFAIK, React intentionally does not interact with properties, as opposed to attributes, for a bunch of different reasons. However, you're still able and encouraged to do the interaction yourself. Just create the video-element without src and then set it "manually" during componentDidMount.

@aweary
Copy link
Contributor

aweary commented Oct 15, 2017

Coming back to this, I agree with @syranide. All of the attributes we set as properties can still be represented as an attribute when doing SSR. In this case, there's no way to serialize the stream for later hydration, so it would extend the set of attributes/properties that we are expected to support.

@chyzwar
Copy link

chyzwar commented Oct 16, 2017

Maybe it would be possible to support srcObject in SSR as well. According to spec srcObject would work with MediaStream, Blob, File. On Server you cannot really use MediaStream and Blob or File can be serialized,

It is kind of interesting idea, server-side rendered video :P
https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement/srcObject

I managed to make it work with a component like this:

import PropTypes from 'prop-types';
import React from 'react';

class Video extends React.Component {
  componentDidMount() {
    this.video.srcObject = this.props.media;
  }

  shouldComponentUpdate(props) {
    return this.props.media !== props.media;
  }

  componentDidUpdate() {
    this.video.srcObject = this.props.media;
  }

  render() {
    const { width, height, muted, children } = this.props;

    return (
      <video
        height={height}
        width={width}
        muted={muted}
        autoPlay
        ref={(video) => { this.video = video; }}
      >
        {children}
      </video>
    );
  }
}

Video.defaultProps = {
  children: null,
  height: 420,
  width: 640,
  muted: true,
  media: null,
};

Video.propTypes = {
  children: PropTypes.element,
  media: PropTypes.shape(
    {
      active: PropTypes.bool,
      ended: PropTypes.bool,
      id: PropTypes.string,
    },
  ),
  height: PropTypes.number,
  width: PropTypes.number,
  muted: PropTypes.bool,
};

export default Video;

It is kinf of ugly and assume silly shouldComponentUpdate,

@simonbuchan
Copy link

SSR seems like a crappy reason to not allow setting properties from react. MediaStreams are very much something your domain/app code could be creating, for example if you're using the ImageCapture API with it you need to get at the video MediaStreamTrack, or you have a preferences dialog to change the camera etc....

Doing all that in react is clumsy and hard to refactor if your UI changes. The obvious answer is to do it in your app and pass it in somehow, this issue means you can use a trivial <video srcObject={stream} /> instead of defining a component wrapper just to pass it in, with all the traps that has (easy to forget property change handling, for example).

Breaking SSR is a valid complaint when:

  • There is a reasonable alternative
  • It would be likely for isomorphic code to accidentally try to render it on the server.

In this situation, both are false.

Incidentally, this issue taught me that react sometimes sets attributes, instead of essentially React.createElement(type, props) => Object.assign(document.createElement(type), props). Why else use htmlFor and className etc.?

@gaearon
Copy link
Collaborator

gaearon commented Jan 5, 2018

As @syranide pointed out there are tons of such properties. It doesn't make sense to me to support just this one and not others. And adding more properties to the whitelist will significantly increase the bundle size of React which is a no-go.

The workaround is straightforward: use refs and assign properties with DOM API directly. If you want, wrap this logic in a custom component and then it'll be an implementation detail.

Thanks for PR though!

@gaearon gaearon closed this Jan 5, 2018
@rfreitas
Copy link

rfreitas commented Jan 9, 2018

what is the purpose of the whitelist? why does react need to have an opinion on the props we pass to the dom?
This breaks a lot of expectations, JSX is meant to look like html, so the expectation is that it works like html.

@rfreitas
Copy link

rfreitas commented Jan 9, 2018

To those interested, preact doesn't seem to block props. If you want good interoperability with React I recommend using preact compat.

@gaearon
Copy link
Collaborator

gaearon commented Jan 9, 2018

@rfreitas If you have a specific suggestion for how properties should work, please file an issue and describe your suggestion. It’s not obvious to me which heuristics Preact is using, and if you find them working well, you can be the ambassador to bring them to React too 😉 Just asking for a single PR to be an exception here is not helpful.

@gaearon
Copy link
Collaborator

gaearon commented Jan 9, 2018

what is the purpose of the whitelist? why does react need to have an opinion on the props we pass to the dom?

Because attributes and properties are different things. Typically people want to set attributes, so we made a change in React 16 to set all unknown props as attributes. You were welcome to participate in all of the public discussions that led to that: #6459 #7311 #10229 #10397 #10385 #10470.

If you disagree with this decision please let us know what exactly you are proposing React should do instead. I am locking this issue because continuing this discussion in a closed PR doesn’t make sense: it will just get lost. A new issue or, even better, an RFC, would be great though. Thanks.

@facebook facebook locked as off-topic and limited conversation to collaborators Jan 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet