Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

AudioSegment.cpp, AudioChannelFormat.cpp and webaudio/AudioBlock.cpp #8

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rtbo
Copy link
Contributor

@rtbo rtbo commented Sep 22, 2017

Hi, a PR with 3 additional cpp files for gecko-media, with reasonable amount of imported headers.

Copy link
Contributor

@philn philn left a comment

Choose a reason for hiding this comment

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

Would it be possible to have a unit-test?

uint32_t sCubebPlaybackLatencyInMilliseconds = 1;
uint32_t sCubebMSGLatencyInFrames = 128;
double sVolumeScale;
uint32_t sCubebPlaybackLatencyInMilliseconds;
Copy link
Contributor

Choose a reason for hiding this comment

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

The changes of this file will break the unit-tests.

"js/HeapAPI.h",
"js/HashTable.h",
Copy link
Contributor

Choose a reason for hiding this comment

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

In my cubeb-rs branch I now get those files from the src dir.

Copy link
Contributor

@cpearce cpearce left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I can't take it as-is, but with modifications, we can accept this.

If you want to discuss your approach, feel free to reach out to me on irc.mozilla.org, my nick is cpearce.

@@ -0,0 +1,57 @@
/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-
Copy link
Contributor

Choose a reason for hiding this comment

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

We really don't want to be pulling in code from the JS engine.

In fact we just merged a PR to remove the existing instances of JS code; d78c69b .

So I don't want to merge this PR while it's importing more JS code. Can you please rebase your commits on master and remove the JS headers?

The goal of this project is to export the core of Firefox's media stacks, into a stand alone crate that builds independently of Servo and Gecko. Servo and Gecko both have a copy of the SpiderMonkey JS engine, we don't want a third here. Anything that sends data to JavaScript needs to do so through an FFI abstraction layer that in turn routes data through to Servo's DOM bindings code. The current plan is to rewrite any WebAudio classes that to talk to JS in Rust, and have those live in Servo's DOM bindings. These objects will need to talk to the corresponding Gecko objects across an FFI barrier.

I haven't looked in detail at how to refactor the WebAudio classes to make this work, we've been focusing on standing up audio and video playback.

What pulls in the JS headers? Is it nsIPrincipal.h? That is a very important class, used to ensure we don't leak data across origins. We need to connect this somehow to Servo's equivalent notion of a principal. So you can probably make progress here by making a copy of this in our gecko/glue dir and stubbing out a base implementation of the functions we call, similar to what I did with the mozilla::Preferences class in c9efca4 . Note at this stage, the stubs don't necessarily need to do anything.

If there's something else that's pulling in the JS code, you can try submitting a patch to Firefox upstream to make it not pull in the JS code (ping me (cpearce) on irc.mozilla.org if you need help with this), or make a copy of the files in our glue dir and modify them to not depend on the JS stuff (which is what I suggested you do for nsIPrincipal).

@nox
Copy link
Contributor

nox commented Jan 8, 2018

This needs to be rebased.

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

Successfully merging this pull request may close these issues.

4 participants