webrtc / samples

WebRTC Web demos and samples
https://webrtc.github.io/samples
BSD 3-Clause "New" or "Revised" License
13.75k stars 5.69k forks source link

getdisplaymedia: add displaysurface #1575

Closed fippo closed 1 year ago

fippo commented 1 year ago

described in https://developer.chrome.com/docs/web-platform/screen-sharing-controls/

@eladalon1983 it seems https://developer.chrome.com/docs/web-platform/screen-sharing-controls/#displaySurface has displaySurface at the wrong level, no?

eladalon1983 commented 1 year ago

@eladalon1983 it seems https://developer.chrome.com/docs/web-platform/screen-sharing-controls/#displaySurface has displaySurface at the wrong level, no?

Yes, that's wrong atm. Thanks for pointing it out! The article will be fixed soon.

beaufortfrancois commented 1 year ago

Thanks @fippo for raising this issue! We're fixing the article in https://github.com/GoogleChrome/developer.chrome.com/pull/3881

eladalon1983 commented 1 year ago

Generally LGTM, but I wonder if adding more than the minimal functionality to an example, might make it less approachable. Please consider if this might deserve its own sample page, possibly along with other similar controls, like in François' page here.

eladalon1983 commented 1 year ago

https://webrtc.github.io/samples/src/content/getusermedia/getdisplaymedia/ is a canonical page for getDisplayMedia. I think it's fine to have displaySurface there.

I think it's fine but sub-optimal. I think ideally, this page should have the minimal required functionality.

beaufortfrancois commented 1 year ago

How about bundling those advanced options like this?

image

     <video id="gum-local" autoplay playsinline muted></video>
+    <fieldset>
+      <legend>Advanced options</legend>
+      <select id="preference" style="display:none">
+        <option value="" selected>Share the default</option>
+        <option value="browser">Share a browser window</option>
+        <option value="window">Share another window</option>
+        <option value="monitor">Share the entire screen</option>
+      </select>
+    </fieldset>
     <button id="startButton" disabled>Start</button>
-    <select id="preference" style="display:none">
-      <option value="" selected>Share the default</option>
-      <option value="browser">Share a browser window</option>
-      <option value="window">Share another window</option>
-      <option value="monitor">Share the entire screen</option>
-    </select>
eladalon1983 commented 1 year ago

How about bundling those advanced options like this? ...

Yeah, that makes sense to me. Especially if the example can still demonstrate that video: true is an option. (I think a lot of developers end up not needing constraints. But I've not checked this hypothesis.)

beaufortfrancois commented 1 year ago

@fippo How does that sound? If that's too much, I can take this PR over. Let me know.

fippo commented 1 year ago

I like the fieldset! Borrowed with updated labels. Also changed the labels to "prefer to share"

beaufortfrancois commented 1 year ago

LGTM with nit

@fippo I'll merge after const startButton = document.getElementById('startButton'); line is restored if that looks good to you.

image

beaufortfrancois commented 1 year ago

And it's live at https://webrtc.github.io/samples/src/content/getusermedia/getdisplaymedia/! Thank you @fippo!

image