fix: video/audio playing on custom stream protocols #22955

pfrazee · 2020-04-03T15:16:46Z

Description of Change

Currently, <video> and <audio> tags fail to play for custom streaming protocols. blink::WebMediaPlayerImpl - which provides the <video> and <audio> behaviors - needs to know whether a data source will stream or fully buffer the response. It determines this behavior with MultibufferDataSource::AssumeFullyBuffered() which has http/s hardwired. An incorrect determination will cause the video/audio to fail playing.

This PR includes a chromium patch which adds a list of "streaming protocols" in order to allow other protocols to register their streaming behavior. MultibufferDataSource::AssumeFullyBuffered() then refers to the registry so that it can correctly determine the data source's settings.

Reference issue: #21018 (comment)

Checklist

PR description included and stakeholders cc'd
npm test passes
tests are changed or added
relevant documentation is changed or added
PR title follows semantic commit guidelines
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Added {stream:} option to registerSchemeAsPrivileged to enable custom protocols to stream video and audio

deepak1556 · 2020-04-03T16:37:47Z

patches/chromium/add_streaming-schemes_registry.patch

+                       &GetSchemeRegistryWithoutLocking()->standard_schemes);
+ }
+
+void AddStreamingScheme(const char* new_scheme) {


Argument SchemeType is not defined here, hence undefined symbols with overloaded functions.

Aha, I'll give that a fix.

deepak1556 · 2020-04-03T16:52:56Z

shell/browser/api/electron_api_protocol.cc

@@ -173,6 +178,11 @@ ProtocolError Protocol::RegisterProtocol(ProtocolType type,
                                         const std::string& scheme,
                                         const ProtocolHandler& handler) {
  bool added = protocol_registry_->RegisterProtocol(type, scheme, handler);
+  if (added) {
+    if (type == ProtocolType::kHttp || type == ProtocolType::kStream) {
+      g_streaming_schemes.push_back(scheme);


Necessary review, this call assumes that registerSchemeAsPrivileged will be called after , but we document users to make the registerSchemeAsPrivileged as the first call in the app as early as possible before app ready due to threading constraints in //url.

The right way would be to provide a new option stream: true in the registerSchemeAsPrivileged api and go from there.

My assumption was that we should automatically register known-streaming protocols, but I guess you're saying that's not an option due to when registerProtocol() is called?

I guess you're saying that's not an option due to when registerProtocol() is called?

Yup

pfrazee · 2020-04-04T21:40:32Z

@deepak1556 The PR has been fixed and updated with your suggestions. You can find a test fiddle here (be sure to update the video path in main.js): https://gist.github.com/283332286b015993700cabf1262ff6d4

Per @nornagon's suggestion, I'm going to submit the chromium patch to chromium to see if they'll upstream it. I'll keep this PR updated with progress on that front.

deepak1556 · 2020-04-04T22:14:02Z

patches/chromium/feat_add_streaming-schemes_registry.patch

+
+This patch adds a list of "streaming protocols" to the url SchemeRegistry in order to allow
+other protocols to register their streaming behavior. MultibufferDataSource::AssumeFullyBuffered()
+then refers to the registry so that it can correctly determine the data source's settings.


Should also provide a way to register via ContentClient::AddAdditionalSchemes https://source.chromium.org/chromium/chromium/src/+/master:content/public/common/content_client.h;l=117-148 so that we can register the scheme in ElectronContentClient::AddAdditionalSchemes that will help the network service process understand the scheme.

This is needed if there is a check in the network service for streaming. I am not familiar with that code, but once you upstream the patch, chromium folks will advice if needed.

pfrazee · 2020-04-09T19:31:56Z

Issue opened in Chromium tracker: https://bugs.chromium.org/p/chromium/issues/detail?id=1068034#c3

pfrazee · 2020-04-22T18:29:43Z

Chromium CI created: https://chromium-review.googlesource.com/c/chromium/src/+/2161417/

ahallicks · 2020-04-23T09:38:23Z

Just to note that the notes read:

Fix to video/audio playblack on custom stream protocols

I think you mean playback.

pfrazee · 2020-04-23T15:23:43Z

Ah, yes. Whoops, thanks for the correction

pfrazee · 2020-04-24T19:13:43Z

Chromium just closed as wontfix https://bugs.chromium.org/p/chromium/issues/detail?id=1068034#c7

nornagon

I'd rather see this accomplished without patching //url. It seems like we could get away with a more specific patch that just targets MultibufferDataSource::AssumeFullyBuffered.

pfrazee · 2020-04-28T22:46:53Z

@nornagon Okay! I've rewritten the PR so that the chromium patch adds the "streaming protocols" list to the media namespace (in the MultibufferDataSource code). Let me know if that's looking better to you.

pfrazee · 2020-04-30T18:13:28Z

Okay looks like the builds are failing because of something related to my patch. I'll check into that.

I'll also add a test and improve the docs a bit.

pfrazee · 2020-04-30T19:41:52Z

Test and more docs are added. Also rebased onto master. Let's see how the builds go...

pfrazee · 2020-04-30T19:43:27Z

A note about the test: I found that very small videos will play correctly even without the fix in this PR. The included video is the smallest I could find which still triggers the behavior (63kb)

shell/renderer/renderer_client_base.cc

patches/chromium/feat_add_streaming-protocol_registry_to_multibuffer_data_source.patch

spec/fixtures/pages/video.html

pfrazee · 2020-05-07T16:26:01Z

Pushed an update as requested, questions remaining on 2 items

pfrazee · 2020-05-08T21:12:51Z

Pushed an update (hopefully) resolving issues

nornagon

Just some minor changes to the patch. Otherwise LGTM! That webm generator is pretty nifty.

docs/api/protocol.md

patches/chromium/feat_add_streaming-protocol_registry_to_multibuffer_data_source.patch

pfrazee · 2020-06-05T17:24:28Z

Hey I hope yall don't mind the bump, just wanted to see if we can get this merged!

nornagon · 2020-06-05T17:26:34Z

@pfrazee looks like there are merge conflicts, would you mind resolving them?

pfrazee · 2020-06-05T17:29:15Z

Sure thing

pfrazee · 2020-06-05T18:08:26Z

Okay still waiting on the tests but I think the conflicts are all set

pfrazee · 2020-06-06T13:39:09Z

Ugh why are the builds failing now. Testing locally.

pfrazee · 2020-06-06T16:28:44Z

Appveyor is failing with

ERROR at //electron/shell/renderer/renderer_client_base.cc:23:11: Can't include this header from here.
#include "media/blink/multibuffer_data_source.h"
          ^------------------------------------
The target:
  //electron:electron_lib
is including a file from the target:
  //media/blink:blink
It's usually best to depend directly on the destination target.

But my build does not. Trying to diagnose...

pfrazee · 2020-06-06T19:00:54Z

Okay I can't sort out why online builds are failing over this but not in my local env. I hate to punt but do yall have any pointers on why this might be happening?

deepak1556 · 2020-06-06T22:37:03Z

@pfrazee for linux and mac we have that failing step you see on appveyor as a separate pipeline which also fails linux-x64-testing-gn-check

The check thats failing is basically https://gn.googlesource.com/gn/+/master/docs/reference.md#cmd_check which is to validate that included headers match the build dependency graph and makes sure that appropriate targets are being built. Its a pre-build sanity check.

If you look at the full output, it reads

ERROR at //electron/shell/renderer/renderer_client_base.cc:23:11: Can't include this header from here.
#include "media/blink/multibuffer_data_source.h"
          ^------------------------------------
The target:
  //electron:electron_lib
is including a file from the target:
  //media/blink:blink

It's usually best to depend directly on the destination target.
In some cases, the destination target is considered a subcomponent
of an intermediate target. In this case, the intermediate target
should depend publicly on the destination to forward the ability
to include headers.

Dependency chain (there may also be others):
  //electron:electron_lib -->
  //content/public/app:app --[private]-->
  //content/public/renderer:renderer_sources --[private]-->
  //content/renderer:renderer --[private]-->
  //media/blink:blink

//media/blink:blink is built via a transitive private dependency, and you are trying to depend on a header from that target directly, its always possible that one of the transitive targets can remove this dependency at some point, at which your compilation is gonna fail. In this case, the fix is add //media/blink:blink to deps of //electron:electron_lib. Ofcourse these errors will always be caught during link step, thats why I said its a pre-build sanity check which is quite useful on a target with nearly 30000 sources.

… player

pfrazee · 2020-06-06T22:57:41Z

@deepak1556 I guess you could say I was in the deps of despair. Appreciate the explanation. I'll look a little more closely next time -- if I had noticed which command that was failing, I might have been able to trace the meaning of the error.

Just pushed again, I'll ping if we're all-clear.

pfrazee · 2020-06-07T01:30:11Z

Appears one (presumably unrelated) test in windows failed due to a timeout. Otherwise all clear.

release-clerk · 2020-06-08T16:49:40Z

Release Notes Persisted

Added {stream:} option to registerSchemeAsPrivileged to enable custom protocols to stream video and audio

pfrazee · 2020-06-08T16:52:10Z

🎉 Thanks again for all the help with this PR

obsius · 2020-06-09T05:34:20Z

I recently ran into this issue on Friday, and what lucky timing with this PR. Now that this code is merged, will it be in the next bugfix release of 9, or will this be scheduled for a later release?

nornagon · 2020-06-09T17:23:36Z

This PR has not been targeted to be backported to any previous version, so with no further action it will be available in v11.

This is a new feature, so backporting it to an old version would mean bumping the minor version and require approval from the Releases WG. See https://github.com/electron/governance/tree/master/wg-releases#feature-backport-requests for more details on the process for requesting feature backports.

pfrazee · 2020-06-16T01:34:16Z

Sorry to be a pest but this didn't make it into today's 10 beta.3, what's the plan? Is it just in the backlog?

pfrazee · 2020-06-16T01:37:27Z

Oh man I'm so sorry, I must have had a cached page or something because I didn't see @nornagon's reponse. I hate being a pest on merges to begin with so now I really feel bad 😅 See you in v11!

… player (electron#22955)

pfrazee requested a review from a team as a code owner April 3, 2020 15:16

electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Apr 3, 2020

deepak1556 reviewed Apr 3, 2020

View reviewed changes

nornagon added the wip ⚒ label Apr 3, 2020

deepak1556 reviewed Apr 3, 2020

View reviewed changes

codebytere changed the title ~~Fix video/audio playing on custom stream protocols~~ fix: video/audio playing on custom stream protocols Apr 3, 2020

electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Apr 4, 2020

pfrazee force-pushed the custom-streaming-protocol-videofix branch from 08c49ae to eb40f0a Compare April 4, 2020 21:33

deepak1556 reviewed Apr 4, 2020

View reviewed changes

nornagon requested changes Apr 27, 2020

View reviewed changes

pfrazee force-pushed the custom-streaming-protocol-videofix branch from 2219d69 to 9024668 Compare April 28, 2020 22:44

pfrazee force-pushed the custom-streaming-protocol-videofix branch 2 times, most recently from f97d7f1 to bd86687 Compare April 30, 2020 19:40

nornagon reviewed May 6, 2020

View reviewed changes

shell/renderer/renderer_client_base.cc Outdated Show resolved Hide resolved

patches/chromium/feat_add_streaming-protocol_registry_to_multibuffer_data_source.patch Outdated Show resolved Hide resolved

spec/fixtures/pages/video.html Outdated Show resolved Hide resolved

pfrazee force-pushed the custom-streaming-protocol-videofix branch from bd86687 to 654ca9d Compare May 7, 2020 16:24

pfrazee force-pushed the custom-streaming-protocol-videofix branch from 654ca9d to c0c20ec Compare May 8, 2020 21:11

nornagon reviewed May 11, 2020

View reviewed changes

pfrazee force-pushed the custom-streaming-protocol-videofix branch from c0c20ec to e68e3c8 Compare May 11, 2020 21:52

pfrazee force-pushed the custom-streaming-protocol-videofix branch from 69a2e97 to 8a5e184 Compare June 5, 2020 17:35

pfrazee force-pushed the custom-streaming-protocol-videofix branch from 8a5e184 to 4120d63 Compare June 6, 2020 15:34

feat: add {stream} opt to custom protocol registry to configure media…

b0234b4

… player

pfrazee force-pushed the custom-streaming-protocol-videofix branch from 4120d63 to b0234b4 Compare June 6, 2020 22:57

nornagon approved these changes Jun 8, 2020

View reviewed changes

nornagon merged commit c6c022d into electron:master Jun 8, 2020

nornagon removed the wip ⚒ label Jun 8, 2020

azeranex mentioned this pull request Jul 1, 2020

Video doesn't work on production build nklayman/vue-cli-plugin-electron-builder#792

Closed

sentialx pushed a commit to sentialx/electron that referenced this pull request Jul 30, 2020

feat: add {stream} opt to custom protocol registry to configure media…

bf1c3a3

… player (electron#22955)

deepak1556 mentioned this pull request Sep 9, 2020

fix: passing content-length down the stream for intercepted requests #25324

Merged

6 tasks

This was referenced Nov 17, 2020

Electron v11, r2-streamer-js HTTP server + prototype no-HTTP streamer (Session.protocol.registerStreamProtocol) edrlab/thorium-reader#1258

Merged

Stream protocols and fs.createReadStream sometimes never complete in Electron 7 #21018

Closed

sentialx pushed a commit to sentialx/electron that referenced this pull request Apr 8, 2021

feat: add {stream} opt to custom protocol registry to configure media…

5f0057f

… player (electron#22955)

codebytere mentioned this pull request Jan 18, 2024

[Bug]: Registered scheme doesn't honor privileges #41032

Open

3 tasks

fix: video/audio playing on custom stream protocols #22955

fix: video/audio playing on custom stream protocols #22955

Conversation

pfrazee commented Apr 3, 2020 • edited by nornagon

Description of Change

Checklist

Release Notes

deepak1556 Apr 3, 2020

Choose a reason for hiding this comment

pfrazee Apr 3, 2020

Choose a reason for hiding this comment

deepak1556 Apr 3, 2020 • edited

Choose a reason for hiding this comment

pfrazee Apr 3, 2020

Choose a reason for hiding this comment

deepak1556 Apr 3, 2020

Choose a reason for hiding this comment

pfrazee commented Apr 4, 2020

deepak1556 Apr 4, 2020 • edited

Choose a reason for hiding this comment

pfrazee Apr 5, 2020

Choose a reason for hiding this comment

pfrazee commented Apr 9, 2020

pfrazee commented Apr 22, 2020

ahallicks commented Apr 23, 2020

pfrazee commented Apr 23, 2020

pfrazee commented Apr 24, 2020

nornagon left a comment

Choose a reason for hiding this comment

pfrazee commented Apr 28, 2020

pfrazee commented Apr 30, 2020

pfrazee commented Apr 30, 2020

pfrazee commented Apr 30, 2020

pfrazee commented May 7, 2020

pfrazee commented May 8, 2020

nornagon left a comment

Choose a reason for hiding this comment

pfrazee commented Jun 5, 2020

nornagon commented Jun 5, 2020

pfrazee commented Jun 5, 2020

pfrazee commented Jun 5, 2020

pfrazee commented Jun 6, 2020

pfrazee commented Jun 6, 2020

pfrazee commented Jun 6, 2020

deepak1556 commented Jun 6, 2020

pfrazee commented Jun 6, 2020

pfrazee commented Jun 7, 2020

release-clerk bot commented Jun 8, 2020

pfrazee commented Jun 8, 2020

obsius commented Jun 9, 2020

nornagon commented Jun 9, 2020

pfrazee commented Jun 16, 2020

pfrazee commented Jun 16, 2020

pfrazee commented Apr 3, 2020 •

edited by nornagon

deepak1556 Apr 3, 2020 •

edited

deepak1556 Apr 4, 2020 •

edited