Closed
Bug 1198458
Opened 9 years ago
Closed 9 years ago
Update WebRTC code to webrtc.org stable branch 43
Categories
(Core :: WebRTC, defect, P1)
Core
WebRTC
Tracking
()
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
backlog | webrtc/webaudio+ |
People
(Reporter: pkerr, Assigned: pkerr)
References
(Depends on 1 open bug)
Details
Attachments
(15 files, 4 obsolete files)
(deleted),
patch
|
jimm
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gcp
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
pehrsons
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
gcp
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jib
:
review+
froydnj
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jib
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
sotaro
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bwc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
pkerr
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gcp
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
pkerr
:
review+
|
Details | Diff | Splinter Review |
Update media/webrtc/trunk with webrtc.org branch 45.
Comment 1•9 years ago
|
||
We're going to update to branch 43 first, since 44 removes a major API we're using.
Summary: Update WebRTC code to webrtc.org stable branch 45 → Update WebRTC code to webrtc.org stable branch 43
Target Milestone: mozilla43 → mozilla44
Comment 2•9 years ago
|
||
This covers the non-simple-merge set of changes for the Windows thread wrappers (we had to merge our addition of ThreadWindowsUI on top of ThreadWindows). Much of the windows-specific part was written by :aklotz; I did some cleanup and also modified RequestCallbackTimer() to let it be called before the thread starts (and hwnd_ is set). This is needed, since we found that InternalInit is beign called on the wrong thread (the parent thread), and deferring it until ::Run() means hwnd_ may be null until then. Try = https://treeherder.mozilla.org/#/jobs?repo=try&revision=6623bbed2e4b
Attachment #8673465 -
Flags: review?(jmathies)
Updated•9 years ago
|
Assignee: pkerr → rjesup
Comment 3•9 years ago
|
||
Comment on attachment 8673465 [details] [diff] [review]
updates to windows threading wrappers to make our UI-thread subclass work on top of 43
Review of attachment 8673465 [details] [diff] [review]:
-----------------------------------------------------------------
::: media/webrtc/trunk/webrtc/system_wrappers/source/thread_win.cc
@@ +217,4 @@
> break;
> +
> + // Alertable sleep to permit RaiseFlag to run and update |stop_|.
> + if (MsgWaitForMultipleObjectsEx(0, nullptr, INFINITE, QS_ALLINPUT, MWMO_ALERTABLE | MWMO_INPUTAVAILABLE) == WAIT_OBJECT_0) {
nit - longer than 80 chars.
Attachment #8673465 -
Flags: review?(jmathies) → review+
Assignee | ||
Comment 4•9 years ago
|
||
Diff from import of webrtc.org 43 branch to current state of the tree with patches applied for the android components. This includes the application of the rollup patch, reject fixes, compile and link fixes, and debug fixes.
Assignee | ||
Comment 5•9 years ago
|
||
Comment on attachment 8676902 [details] [diff] [review]
Comprehensive diff of android components imported from webrtc.org/43
GCP: can you take a look at the changes made to get the android components of the webrtc.org 43 import to build and run. I created an overall diff with all the component patches applied from patch queue: http://hg.mozilla.org/users/paulrkerr_gmail.com/webrtc43_merge.
Attachment #8676902 -
Flags: feedback?(gpascutto)
Comment 6•9 years ago
|
||
Attachment #8677052 -
Flags: review?(pehrsons)
Comment 7•9 years ago
|
||
Comment on attachment 8677052 [details] [diff] [review]
Delay sending a frame in webrtc when reconfiguring the codec
Review of attachment 8677052 [details] [diff] [review]:
-----------------------------------------------------------------
Some questions and minor things, but in general it looks good.
::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +1058,5 @@
> }
>
> // XXX we need to figure out how to feed back changes in preferred capture
> +// resolution to the getUserMedia source.
> +// Returns boolean if we've submitted an async change (and took ownership
Returns what when? I only see it ever returning false (though lines 1074-1153 are cut out).
@@ +1159,5 @@
> if (changed) {
> // On a resolution change, bounce this to the WebRTC thread to re-configure. Do *not* block
> // the calling thread since that may be the MSG thread.
> // XXX We probably should queue frames until this is complete, at least on the first frame of
> // a call, to avoid immediate resolution changes, or throw away frames (ugh)
This XXX can be removed now.
@@ +1165,5 @@
> + // We can't pass a UniquePtr<> or unique_ptr<> to a lambda directly
> + webrtc::I420VideoFrame *new_frame = nullptr;
> + if (frame) {
> + new_frame = new webrtc::I420VideoFrame();
> + new_frame->ShallowCopy(*frame);
Who has ownership of the data after this ShallowCopy? I assume it just copies the pointer to the data, so can we be sure it'll still be alive when we need it?
@@ +1202,5 @@
> } // else no change; mSendingWidth likely was 0
> + if (local_frame) {
> + self->mPtrExtCapture->IncomingFrame(*local_frame);
> + self->mVideoCodecStat->SentFrame();
> + CSFLogDebug(logTag, "%s Inserted a frame", __FUNCTION__);
Mention that this frame came from the async reconfig so we can keep them apart in the log.
@@ +1211,4 @@
> CSFLogDebug(logTag, "%s: proxying lambda to WebRTC thread for reconfig (width %u/%u, height %u/%u",
> __FUNCTION__, width, mLastWidth, height, mLastHeight);
> mThread->Dispatch(webrtc_runnable.forget(), NS_DISPATCH_NORMAL);
> // XXX we should queue the frame or something if we do this
This can be removed now.
@@ +1324,5 @@
> + // insert the frame to video engine in I420 format only
> + MOZ_ASSERT(mPtrExtCapture);
> +
> + webrtc::I420VideoFrame i420_frame;
> + // B2G feeds us frames in YV12, for example
We're doing a bunch of conversions in MediaPipeline.cpp, can the YV12 stuff be moved there? It would be nice to keep it all in one place.
Attachment #8677052 -
Flags: review?(pehrsons) → review+
Comment 8•9 years ago
|
||
Moved conversion to MediaPipeline; cleanup; fix return value bug
Attachment #8677243 -
Flags: review?(pehrsons)
Updated•9 years ago
|
Attachment #8677052 -
Attachment is obsolete: true
Comment 9•9 years ago
|
||
Comment on attachment 8677243 [details] [diff] [review]
Delay sending a frame in webrtc when reconfiguring the codec
Review of attachment 8677243 [details] [diff] [review]:
-----------------------------------------------------------------
::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +1058,5 @@
> }
>
> // XXX we need to figure out how to feed back changes in preferred capture
> +// resolution to the getUserMedia source.
> +// Returns boolean if we've submitted an async change (and took ownership
"Returns true if"
Attachment #8677243 -
Flags: review?(pehrsons) → review+
Comment 10•9 years ago
|
||
:jesup asked me to do some testing (>2 weeks ago), so here's the result. No issues from running webrtc-landing demos but I don't have webcam/microphone.
Comment 11•9 years ago
|
||
(In reply to Jan Beich from comment #10)
> Created attachment 8678398 [details] [diff] [review]
> Unbreak build on BSDs by fixing non-POSIX thread includes/usage (tested only
> FreeBSD)
>
> :jesup asked me to do some testing (>2 weeks ago), so here's the result. No
> issues from running webrtc-landing demos but I don't have webcam/microphone.
Thanks! At the minimum that nails the silly compile issues, and likely it will work fully.
Comment 12•9 years ago
|
||
(In reply to Paul Kerr [:pkerr] from comment #5)
> GCP: can you take a look at the changes made to get the android components
> of the webrtc.org 43 import to build and run. I created an overall diff with
> all the component patches applied from patch queue:
> http://hg.mozilla.org/users/paulrkerr_gmail.com/webrtc43_merge.
FWIW I did a code review a while ago and only a single thing jumped out at me, which was reported on IRC.
From testing, basic gUM seems to work correctly everywhere, including non-NEON support. But I can't get talky.io working with this patchset (on Android, might affect desktop too).
Updated•9 years ago
|
Attachment #8676902 -
Attachment is patch: true
Attachment #8676902 -
Attachment mime type: text/x-patch → text/plain
Updated•9 years ago
|
Attachment #8676902 -
Flags: feedback?(gpascutto) → feedback+
Assignee | ||
Comment 13•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Assignee: rjesup → pkerr
Assignee | ||
Comment 14•9 years ago
|
||
New thread creation timing in webrtc.org code was causing a second LoadMonitor
instance to be created in AddObserver method.
Assignee | ||
Comment 15•9 years ago
|
||
Assignee | ||
Comment 16•9 years ago
|
||
Assignee | ||
Comment 17•9 years ago
|
||
Assignee | ||
Comment 18•9 years ago
|
||
Assignee | ||
Comment 19•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8686761 -
Flags: review?(jib)
Assignee | ||
Updated•9 years ago
|
Attachment #8686763 -
Flags: review?(gpascutto)
Assignee | ||
Updated•9 years ago
|
Attachment #8686765 -
Flags: review?(jib)
Assignee | ||
Comment 20•9 years ago
|
||
Comment on attachment 8686767 [details] [diff] [review]
ensure SetAndroidObjects is called on main thread
gcp: can you look at the change that I made to VideoConduit.cpp and verify that making the SetAndroidObjects call on the main thread will work.
Attachment #8686767 -
Flags: review?(gpascutto)
Assignee | ||
Updated•9 years ago
|
Attachment #8686770 -
Flags: review?(jib)
Assignee | ||
Updated•9 years ago
|
Attachment #8686771 -
Flags: review?(jib)
Assignee | ||
Updated•9 years ago
|
Attachment #8686772 -
Flags: review?(gpascutto)
Comment 21•9 years ago
|
||
Comment on attachment 8686761 [details] [diff] [review]
shutdown backend on MediaManager thread
I believe I already reviewed this patch in bug 1218799 comment 10. Would like to see those comments addressed for r+.
Attachment #8686761 -
Flags: review?(jib)
Comment 22•9 years ago
|
||
Comment on attachment 8686765 [details] [diff] [review]
ensure registration of external codec occurs on VideoConduit.mThread
Review of attachment 8686765 [details] [diff] [review]:
-----------------------------------------------------------------
::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +1230,5 @@
>
> MediaConduitErrorCode
> WebrtcVideoConduit::SetExternalSendCodec(VideoCodecConfig* config,
> VideoEncoder* encoder) {
> + if (!OnThread(mThread)) {
Where is mThread? I don't see it in any of the preceding patches or in
http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
Comment 23•9 years ago
|
||
Comment on attachment 8686770 [details] [diff] [review]
add check of threadpool timeout
Review of attachment 8686770 [details] [diff] [review]:
-----------------------------------------------------------------
::: xpcom/threads/nsThreadPool.cpp
@@ +184,5 @@
> } else {
> if (wasIdle) {
> // if too many idle threads or idle for too long, then bail.
> + if (mIdleCount > mIdleThreadLimit ||
> + (mIdleThreadTimeout != UINT32_MAX && (now - idleSince) >= timeout)) {
Why is this needed? I find no code that sets mIdleThreadTimeout to UINT32_MAX.
Comment 24•9 years ago
|
||
Comment on attachment 8686771 [details] [diff] [review]
set and use threadpool timeout
Review of attachment 8686771 [details] [diff] [review]:
-----------------------------------------------------------------
::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +97,5 @@
> // shared pool, max 1 thread
> nsRefPtr<SharedThreadPool>
> + threadpool = SharedThreadPool::Get(NS_LITERAL_CSTRING("WebRTC"), 1);
> + mThread = threadpool.get();
> + threadpool->SetIdleThreadTimeout(UINT32_MAX);
That's what I get for reviewing patches in order. Never timing out seems odd. A comment on why this is needed would be great.
Attachment #8686771 -
Flags: review?(jib) → review+
Updated•9 years ago
|
Attachment #8686770 -
Flags: review?(jib) → review+
Comment 25•9 years ago
|
||
Comment on attachment 8686770 [details] [diff] [review]
add check of threadpool timeout
Review of attachment 8686770 [details] [diff] [review]:
-----------------------------------------------------------------
::: xpcom/threads/nsThreadPool.cpp
@@ +184,5 @@
> } else {
> if (wasIdle) {
> // if too many idle threads or idle for too long, then bail.
> + if (mIdleCount > mIdleThreadLimit ||
> + (mIdleThreadTimeout != UINT32_MAX && (now - idleSince) >= timeout)) {
Actually, when I look at this again this change looks like a no-op. If mIdleThreadTimeout is UINT32_MAX then timeout will also be UINT32_MAX according to [1], which means that (now - idleSince) will never be bigger than it.
So this doesn't seem necessary to me. Am I missing something?
[1] http://mxr.mozilla.org/mozilla-central/source/js/src/vm/PosixNSPR.cpp#324
Comment 26•9 years ago
|
||
(In reply to Jan-Ivar Bruaroey [:jib] from comment #25)
> Comment on attachment 8686770 [details] [diff] [review]
> add check of threadpool timeout
>
> Review of attachment 8686770 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: xpcom/threads/nsThreadPool.cpp
> @@ +184,5 @@
> > } else {
> > if (wasIdle) {
> > // if too many idle threads or idle for too long, then bail.
> > + if (mIdleCount > mIdleThreadLimit ||
> > + (mIdleThreadTimeout != UINT32_MAX && (now - idleSince) >= timeout)) {
>
> Actually, when I look at this again this change looks like a no-op. If
> mIdleThreadTimeout is UINT32_MAX then timeout will also be UINT32_MAX
> according to [1], which means that (now - idleSince) will never be bigger
> than it.
no, but we exit on (now-idleSince) == timeout as well, and that can happen (idleSince = 0, now = UINT32_MAX means (now-idleSince) == UINT32_MAX).
Also, this is somewhat less brittle than relying on a side-effect (which we can't anyways without changing >= to >).
Comment 27•9 years ago
|
||
Comment on attachment 8686770 [details] [diff] [review]
add check of threadpool timeout
Review of attachment 8686770 [details] [diff] [review]:
-----------------------------------------------------------------
This needs an xpcom peer review. (I have talked this over with Nathan on IRC a while back)
Attachment #8686770 -
Flags: review?(nfroyd)
Comment 28•9 years ago
|
||
Comment on attachment 8686770 [details] [diff] [review]
add check of threadpool timeout
Review of attachment 8686770 [details] [diff] [review]:
-----------------------------------------------------------------
r+ to Jan-Ivar for his review comments, too!
Attachment #8686770 -
Flags: review?(nfroyd) → review+
Comment 29•9 years ago
|
||
Comment on attachment 8686763 [details] [diff] [review]
create LoadMonitor only when LoadManager singleton is created
Review of attachment 8686763 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/media/systemservices/LoadManager.cpp
@@ +51,5 @@
> LOG(("LoadManager - Initializing (%dms x %d, %f, %f)",
> mLoadMeasurementInterval, mAveragingMeasurements,
> mHighLoadThreshold, mLowLoadThreshold));
> MOZ_ASSERT(mHighLoadThreshold > mLowLoadThreshold);
> +#if 1 //DEBUG(pkerr)
*cough*
::: dom/media/systemservices/LoadMonitor.cpp
@@ +629,5 @@
> {
> LOG(("Initializing LoadMonitor"));
>
> RefPtr<RTCLoadInfo> load_info = new RTCLoadInfo();
> + if (!load_info) {
This is dead code. new is by default infallible, so you can't ever have the error case run.
Attachment #8686763 -
Flags: review?(gpascutto) → review-
Comment 30•9 years ago
|
||
Comment on attachment 8686767 [details] [diff] [review]
ensure SetAndroidObjects is called on main thread
Review of attachment 8686767 [details] [diff] [review]:
-----------------------------------------------------------------
The patch description is deceptive, there's more here.
::: media/webrtc/trunk/webrtc/modules/video_capture/video_capture_impl.cc
@@ +261,5 @@
> // SetApplyRotation doesn't take any lock. Make a local copy here.
> bool apply_rotation = apply_rotation_;
> + int target_width;
> + int target_height;
> +
nit: spurious whitespace
::: media/webrtc/trunk/webrtc/voice_engine/voice_engine_impl.cc
@@ +175,5 @@
> } else {
> #if !defined(WEBRTC_GONK) && defined(ANDROID)
> AudioDeviceInstanceJni::ClearAndroidAudioDeviceObjects();
> #endif
> +#ifdef WEBRTC_ANDROID_OPENSLES
Can you check that this define actually gets set, before landing?
Attachment #8686767 -
Flags: review?(gpascutto) → review+
Comment 31•9 years ago
|
||
Comment on attachment 8686772 [details] [diff] [review]
add ownership check to OMX reservation logic
Review of attachment 8686772 [details] [diff] [review]:
-----------------------------------------------------------------
I can't even pretend to be a peer of this code, forwarding review.
Attachment #8686772 -
Flags: review?(gpascutto) → review?(jolin)
Comment 32•9 years ago
|
||
Comment on attachment 8686772 [details] [diff] [review]
add ownership check to OMX reservation logic
Review of attachment 8686772 [details] [diff] [review]:
-----------------------------------------------------------------
Sotaro did this work, so he's the right reviewer.
Attachment #8686772 -
Flags: review?(jolin) → review?(sotaro.ikeda.g)
Comment 33•9 years ago
|
||
Comment on attachment 8686772 [details] [diff] [review]
add ownership check to OMX reservation logic
Review of attachment 8686772 [details] [diff] [review]:
-----------------------------------------------------------------
review+ if the comment is addressed.
::: dom/media/omx/OMXCodecWrapper.cpp
@@ +70,5 @@
> {
> if (!mClient) {
> return;
> }
> + //CODEC_ERROR("OMX Reservation: Releasing resource: (%d) %s", (int) mType, mOwned ? "Owned" : "Not owned");
// You might want to add a comment about the following code to make clear the intent.
Updated•9 years ago
|
Attachment #8686772 -
Flags: review?(sotaro.ikeda.g) → review+
Comment 34•9 years ago
|
||
Comment on attachment 8686765 [details] [diff] [review]
ensure registration of external codec occurs on VideoConduit.mThread
Clearing r? since bugzilla is bugging me. Waiting on answer to question in comment 22.
Attachment #8686765 -
Flags: review?(jib)
Comment 35•9 years ago
|
||
This leaves the Init() and ConfigureSend/RecvCodec(s) and such on MainThread for now (we'll do a followup to try to get them off it). As such, on a mid-call reconfig (resolution change, etc) we have to execute it on MainThread. This is far safer than trying to rework init to be async or using non-event-loop-spinning sync dispatches
Adds code to drop frames in the "wrong" resolution until the reconfig is done (which should be fast unless MainThread is hosed).
Note I left some of the (earlier in the queue) work splitting up Init() and ~/Destroy() because we'll likely use it once we manage to get off mainthread.
Attachment #8688123 -
Flags: review?(docfaraday)
Comment 36•9 years ago
|
||
Comment on attachment 8688123 [details] [diff] [review]
deadlock.patch
Review of attachment 8688123 [details] [diff] [review]:
-----------------------------------------------------------------
::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +1191,5 @@
> + // MUST run on the same thread as Init()/etc
> + if (!NS_IsMainThread()) {
> + NS_DispatchToMainThread(webrtc_runnable.forget());
> + } else {
> + webrtc_runnable->Run();
If we run this in-place, it'll deadlock, right?
::: media/webrtc/signaling/src/media-conduit/VideoConduit.h
@@ +360,5 @@
> RecvCodecList mRecvCodecList;
>
> Mutex mCodecMutex; // protects mCurrSendCodecConfig
> nsAutoPtr<VideoCodecConfig> mCurSendCodecConfig;
> + bool mInReconfig;
We probably want to make this atomic, since we're using it on multiple threads.
Comment 37•9 years ago
|
||
>
> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.h
> @@ +360,5 @@
> > RecvCodecList mRecvCodecList;
> >
> > Mutex mCodecMutex; // protects mCurrSendCodecConfig
> > nsAutoPtr<VideoCodecConfig> mCurSendCodecConfig;
> > + bool mInReconfig;
>
> We probably want to make this atomic, since we're using it on multiple
> threads.
Scratch that, I think we're locking everywhere we touch this...
Comment 38•9 years ago
|
||
> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
> @@ +1191,5 @@
> > + // MUST run on the same thread as Init()/etc
> > + if (!NS_IsMainThread()) {
> > + NS_DispatchToMainThread(webrtc_runnable.forget());
> > + } else {
> > + webrtc_runnable->Run();
>
> If we run this in-place, it'll deadlock, right?
If we run it off-mainthread (off thread used for Init()), it'll assert in debug builds in the webrtc.org code, and in non-debug builds you're in danger because the datastructures aren't locked anymore.
Assignee | ||
Comment 39•9 years ago
|
||
(In reply to Jan-Ivar Bruaroey [:jib] from comment #22)
> Comment on attachment 8686765 [details] [diff] [review]
> ensure registration of external codec occurs on VideoConduit.mThread
>
> Review of attachment 8686765 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
> @@ +1230,5 @@
> >
> > MediaConduitErrorCode
> > WebrtcVideoConduit::SetExternalSendCodec(VideoCodecConfig* config,
> > VideoEncoder* encoder) {
> > + if (!OnThread(mThread)) {
>
> Where is mThread? I don't see it in any of the preceding patches or in
> http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/
> media-conduit/VideoConduit.cpp
It is defined in VideoConduit.h.
Comment 40•9 years ago
|
||
> > > + if (!OnThread(mThread)) {
> >
> > Where is mThread? I don't see it in any of the preceding patches or in
> > http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/
> > media-conduit/VideoConduit.cpp
>
> It is defined in VideoConduit.h.
And in the lastest patch to to bwc I remove mThread (for now).
Comment 41•9 years ago
|
||
(In reply to Randell Jesup [:jesup] from comment #38)
> > ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
> > @@ +1191,5 @@
> > > + // MUST run on the same thread as Init()/etc
> > > + if (!NS_IsMainThread()) {
> > > + NS_DispatchToMainThread(webrtc_runnable.forget());
> > > + } else {
> > > + webrtc_runnable->Run();
> >
> > If we run this in-place, it'll deadlock, right?
>
> If we run it off-mainthread (off thread used for Init()), it'll assert in
> debug builds in the webrtc.org code, and in non-debug builds you're in
> danger because the datastructures aren't locked anymore.
Sure, but we may need to unwind the stack (instead of just doing webrtc_runnable->Run()), since mCodecMutex is going to already be locked. Or maybe we can avoid the lock by not using a runnable if we're already on main (and moving the code in the lambda into a named function we can call).
Comment 42•9 years ago
|
||
(In reply to Byron Campen [:bwc] from comment #41)
> Sure, but we may need to unwind the stack (instead of just doing
> webrtc_runnable->Run()), since mCodecMutex is going to already be locked. Or
> maybe we can avoid the lock by not using a runnable if we're already on main
> (and moving the code in the lambda into a named function we can call).
Ah, good point (though for now the on-main-thread code is there mostly for completeness). Probably cleaner/easier to read with a discrete function anyways.
Comment 43•9 years ago
|
||
Moved innards of lambda to a method so we can call it under the lock if we're on the right thread already
Attachment #8688123 -
Attachment is obsolete: true
Attachment #8688123 -
Flags: review?(docfaraday)
Attachment #8688262 -
Flags: review?(docfaraday)
Comment 44•9 years ago
|
||
This fixes display of video from OpenH264 - for 640x480, OpenH264 returns 640x480 in a 704 stride buffer. Before 43, the code called ExtractBuffer() (which understands strides) and copied to a fresh contiguous buffer. Now it prefers to avoid copying, so we needed to teach the VideoConduit/MediaPipeline code about strides. H264 video looks good now
Attachment #8688264 -
Flags: review?(pkerr)
Updated•9 years ago
|
Assignee: pkerr → rjesup
Comment 45•9 years ago
|
||
Updated for review comments
Attachment #8688265 -
Flags: review?(gpascutto)
Updated•9 years ago
|
Attachment #8686763 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8688265 -
Flags: review?(gpascutto) → review+
Comment 46•9 years ago
|
||
Attachment #8688488 -
Flags: review?(nfroyd)
Comment 47•9 years ago
|
||
Comment on attachment 8688488 [details] [diff] [review]
add rtc:: to suppressions since upstream uses that for generic stuff imported from Chromium/etc
Review of attachment 8688488 [details] [diff] [review]:
-----------------------------------------------------------------
r=me assuming the webrtc:: namespace is still used upstream. If not, we should delete the webrtc check.
Attachment #8688488 -
Flags: review?(nfroyd) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8688264 -
Flags: review?(pkerr) → review+
Updated•9 years ago
|
backlog: --- → webrtc/webaudio+
Rank: 15
Updated•9 years ago
|
Attachment #8688262 -
Flags: review?(docfaraday) → review+
Assignee | ||
Comment 48•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Assignee: rjesup → pkerr
Assignee | ||
Comment 49•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8689152 -
Attachment is obsolete: true
Assignee | ||
Comment 50•9 years ago
|
||
Comment on attachment 8689153 [details] [diff] [review]
prevent generation of multiple jni global refs
r+ from gcp via #media irc.
Attachment #8689153 -
Flags: review+
Comment 51•9 years ago
|
||
Comment 52•9 years ago
|
||
Comment 53•9 years ago
|
||
Comment 54•9 years ago
|
||
bugherder |
Updated•9 years ago
|
status-firefox43:
affected → ---
Target Milestone: mozilla44 → mozilla45
Updated•8 years ago
|
Blocks: webrtc_updates
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•