Closed
Bug 1268984
Opened 8 years ago
Closed 8 years ago
WebRTC H264 video rendering issue
Categories
(Core :: Audio/Video: GMP, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox46 | --- | unaffected |
firefox47 | + | fixed |
firefox48 | --- | fixed |
firefox49 | --- | verified |
People
(Reporter: drno, Assigned: cpearce)
References
Details
(Keywords: regression)
Attachments
(6 files)
181.39 KB,
application/octet-stream
|
Details | |
191.81 KB,
application/octet-stream
|
Details | |
3.93 KB,
text/plain
|
Details | |
58 bytes,
text/x-review-board-request
|
jesup
:
review+
mozbugz
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details |
58 bytes,
text/x-review-board-request
|
mozbugz
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jesup
:
review+
|
Details |
Since a couple of days we are observing the following problems when trying to use Cisco Spark for 1:1 video calls: - Initially you see your own preview video - your own preview video might freeze later (but not always?) - The remote video never gets rendered, you only get a gray rendering box instead OR You see an initial split second of video which then freezes - The Spark Web UI might freeze as in the call timer no longer updates and you can't click any of the buttons in the UI, e.g. to terminate the call - If your UI froze it takes a very long time until Spark realizes the call failure and hangs up - by then the other side has usually already hung up - When doing the very first call with a freshly started Firefox calls appear to work normally, but the second and all subsequent calls are broken - This affects Mozilla's official builds, but some developers have reported that their locally compiled builds are not affected by this (?)
Reporter | ||
Updated•8 years ago
|
backlog: --- → webrtc/webaudio+
Rank: 19
Reporter | ||
Comment 1•8 years ago
|
||
This was DevEdition (47.0a2 (2016-04-25)) on my Mac. The UI froze. And it only rendered the grey box. From looking into the trace it appears that it initially send to send a few RTP video packet up to packet #40. But then stops sending anything for the next 51 seconds. And then sends out 3 RTCP sender reports before continuing with the H264 RTP video as if nothing had happened (in terms of RTP sequence numbers).
Reporter | ||
Comment 2•8 years ago
|
||
This is other side of the test call on a Linux machine, which did not freeze. This side looks more sane. Sending video consistently, and receives 20 packet of video, packets #166 - #198. It appears to keep sending video until the UI probably gave up on the call and hung up from the application logic, because it wasn't receiving anything from the other side.
Comment 3•8 years ago
|
||
Michael, Nils -- Can you capture here what we know (and I realize it may not be much yet) about this problem? I'd like to chat on irc on Monday about how to attack this and what our next steps should be. Enjoy your weekend!
Flags: needinfo?(mfroman)
Flags: needinfo?(drno)
Reporter | ||
Comment 4•8 years ago
|
||
I tried to capture everything I know of in the initial report message (besides the additional information regarding the packet traces). I think the next step from our side could/should be to run with WebRTC logs turned on and check what the media logs tell us about this.
Flags: needinfo?(drno)
Reporter | ||
Updated•8 years ago
|
Reporter | ||
Updated•8 years ago
|
status-firefox46:
--- → affected
Reporter | ||
Comment 5•8 years ago
|
||
As a whole range of Firefox version appears to be affected I'm wondering if it is not the Firefox version but maybe the version of the Open H.264 plugin causing this. But I can't recall that we recently shipped any update of the plugin, or Maire?
Flags: needinfo?(mreavy)
Reporter | ||
Comment 6•8 years ago
|
||
Actually the 46 was the version on my Linux system, which might simply suffer from the Mac side freezing up and not sending anything.
Reporter | ||
Comment 7•8 years ago
|
||
Have we seen anything else then Mac suffering from this problem?
Comment 8•8 years ago
|
||
(In reply to Nils Ohlmeier [:drno] from comment #5) > As a whole range of Firefox version appears to be affected I'm wondering if > it is not the Firefox version but maybe the version of the Open H.264 plugin > causing this. But I can't recall that we recently shipped any update of the > plugin, or Maire? Our last update to the plugin was in January.
Flags: needinfo?(mreavy)
Comment 9•8 years ago
|
||
(In reply to Maire Reavy [:mreavy] (Plz ni?:mreavy) from comment #3) > Michael, Nils -- Can you capture here what we know (and I realize it may not > be much yet) about this problem? > > I'd like to chat on irc on Monday about how to attack this and what our next > steps should be. Enjoy your weekend! I've tried this with local builds (both debug and opt), and it works perfectly. I've tried it with official Nightly downloads and it always fails on the 2nd+ call in the way Nils described. It either freezes or only shows grey video for the remote side. Adam proposed it is something build-related. I've cobbled together a mozconfig that should be close to the config used for the nightly builds, but I haven't been able to test it yet.
Flags: needinfo?(mfroman)
Comment 10•8 years ago
|
||
Michael -- Since you are closest to this problem atm, I'd like to assign this to you. Can you test out abr's theory and report back what you find? Nils and I are available for additional kibitzing -- perhaps after you've had a chance to test the latest mozconfig (that's close to Nightly).
Assignee: nobody → mfroman
Comment 11•8 years ago
|
||
Update: the close-to-nightly mozconfig exhibits the grey video and/or freeze behavior just like official Nightly.
Comment 12•8 years ago
|
||
I used this file to build locally something that was very close to Nightly. With this file as is, I see the bug. If I comment out the following line the grey video/freeze issue goes away: ac_add_options --enable-eme=widevine
Reporter | ||
Comment 13•8 years ago
|
||
A little easier STR: - set the following user pref in both Firefox instances you use for testing: media.navigator.video.preferred_codec = 126 Note1: this is a hidden pref, so you need to add it as a new pref in about:config Note2: theoretically it should enough to set this option only for one side, the one who creates the SDP offer, but it is often not trivial to figure out who that is going to be, thus easier to do it on both sides - go to your preferred WebRTC based calling service, e.g. appear.in, talky.io - connect the two browsers in one call Note: as described above it will probably work for the first call, so probably need to exit the first call and then try again
Reporter | ||
Updated•8 years ago
|
Summary: Cisco Spark video rendering issue → WebRTC video rendering issue
Reporter | ||
Comment 14•8 years ago
|
||
So with the steps from comment #13 I did a test call between 48 and 49 on my MacBook. Both with the Widevine plugin enabled: - one side the whole webpage freezes for a couple of seconds Disabled widevine plugin on 49: - webpage froze on 48 and 49 showed a frozen one picture of the remote side, but the local preview window is still alive Disabled widevine plugin on both: - call works as expected on both ends
Reporter | ||
Comment 15•8 years ago
|
||
So known workaround: disable the Widevine plugin on about:addons Note: once someone experience the above described freeze, it blocks also the rendering in other/new tabs until the frozen tabs thaws
Summary: WebRTC video rendering issue → WebRTC H264 video rendering issue
Assignee | ||
Comment 16•8 years ago
|
||
The GMP which GeckoMediaPluginServiceParent::FindPluginForAPIFrom() returns depends on the order in which GMPs lie in GMPServiceParent::mPlugins. However when we shutdown a GMPParent we remove and then re-append the GMPParent to mPlugins. This means the order in which GMPs lie in the list changes. So when WebRTC requests an H.264 decoder, the first time it will get OpenH264, since that's first in the list. But once we dispose of that decoder, its GMPParent will be cloned and the clone will be appended to the end of the list. This means the next time WebRTC requests a decoder, it'll get whatever was next in the list. This could be the Adobe GMP, which seems to be able to handle whatever WebRTC is putting into it. However, if you do this enough times, you'll get the Widevine CDM, which can't handle whatever WebRTC is putting into it. So a quick hack to fix this is in ReAddOnGMPThread is to re-insert the clone of the GMP into the slot in mPlugins that the original occupied. Then WebRTC will always get OpenH264 whenever it requests for an H.264 decoder, as the order of the GMPParents in mPlugins won't change. Review commit: https://reviewboard.mozilla.org/r/50297/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50297/
Comment 17•8 years ago
|
||
Hmm... The long-term fix seems to be to distinguish between real-time and non-real time decoders and have Adobe and WideVine return negative to that query
Comment 18•8 years ago
|
||
Comment on attachment 8748450 [details] MozReview Request: Bug 1268984 - Ensure GMPs are re-inserted in GMPServiceParent::mPlugins in the same order in ReAddOnGMPThread. r=jesup https://reviewboard.mozilla.org/r/50297/#review47117 We can just comment the first issue I think; if so this is all nits we can resolve and land ::: dom/media/gmp/GMPServiceParent.cpp:1017 (Diff revision 1) > - GMPParent* clone = ClonePlugin(gmpToClone); > + RefPtr<GMPParent> clone = ClonePlugin(gmpToClone); > + { > + MutexAutoLock lock(mMutex); > + mPlugins.AppendElement(clone); > + } > if (!aNodeId.IsEmpty()) { > clone->SetNodeId(aNodeId); > } > return clone; the use of RefPtr<> here is confusing, as we return it as a raw ptr. I presume mPlugins is a TArray<RefPtr<...>>? Otherwise when we return clone, we drop the ref held by clone, and it gets deleted... Perhaps this should return already_AddRefed like ClonePlugin? Otherwise, if this is the desired pattern, comment the lifetime issues ::: dom/media/gmp/GMPServiceParent.cpp:1207 (Diff revision 1) > + MutexAutoLock lock(mMutex); > + if (mPlugins.Contains(aOld)) { > + mPlugins[mPlugins.IndexOf(aOld)] = gmp; > - } > + } Does it make any sense where to complain if Contains() fails?
Attachment #8748450 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Assignee: mfroman → cpearce
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8748450 [details] MozReview Request: Bug 1268984 - Ensure GMPs are re-inserted in GMPServiceParent::mPlugins in the same order in ReAddOnGMPThread. r=jesup Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50297/diff/1-2/
Assignee | ||
Comment 20•8 years ago
|
||
https://reviewboard.mozilla.org/r/50297/#review47117 I changed to have our SelectGMP/FindAPI return already_AddRefed. > the use of RefPtr<> here is confusing, as we return it as a raw ptr. I presume mPlugins is a TArray<RefPtr<...>>? Otherwise when we return clone, we drop the ref held by clone, and it gets deleted... > Perhaps this should return already_AddRefed like ClonePlugin? Otherwise, if this is the desired pattern, comment the lifetime issues I will make SelectPluginForAPI() return an already_AddRefed<GMPParent>. > Does it make any sense where to complain if Contains() fails? I can assert that aOld is in mPlugins; it's supposed to be. I just added the Contains() check because I'm paranoid.
Assignee | ||
Updated•8 years ago
|
backlog: webrtc/webaudio+ → ---
Component: WebRTC → Audio/Video: GMP
Comment 22•8 years ago
|
||
FYI, it's not clear but that was r+ with no comments. re: ekr's comment, I agree, and was saying so in IRC - we should distinguish in GMP definitions between realtime-capable and non (and also profiles, so it doesn't see OpenH264 as viable for High profile).
Comment 23•8 years ago
|
||
Try failure in GTEST: TEST-UNEXPECTED-FAIL | GeckoMediaPlugins.GMPCrossOrigin | Value of: mGMP && (mGMP->GetPluginId() == aGMP->GetPluginId()) == mShouldBeEqual
Comment 24•8 years ago
|
||
If this is fallout from enabling Widevine, we'll want to uplift this fix to Beta 47 because we plan to ship Widevine in 47.
Blocks: widevine-uplift
Keywords: regression
Updated•8 years ago
|
Rank: 19 → 5
Assignee | ||
Comment 25•8 years ago
|
||
The test is failing because we store the GMP storage records for private browsing mode in memory on the GMPStorageParent, which is owned by the GMPParent, and when the GMPParent dies, we'll blow away the records. The test is racy; the test opens multiple GMPParents of the same origin, and some sometimes we'll end up deleting all references to the GMPParent but expect its storage to persist into the next same-origin GMPParent. I think it makes sense for GMP storage to be persistent in the same origin in the same PB session across GMPParent instances. So we should put the storage somewhere more permanent, maybe on the GMPService, and clear that when "browser:purge-session-history" fires.
Assignee | ||
Comment 26•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8240fb7fb5ae
See Also: → 1266336
Assignee | ||
Comment 27•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b685687d443
Assignee | ||
Comment 28•8 years ago
|
||
Prior to this change, we'd store the GMPStorage records for private browsing sessions in the GMPStorageParent. The problem with this is that they only have a lifespan matching their corresponding GMPParent. This means that if a GMP stores something in a PB session, and the GMP is shutdown and then re-created, we are likely to loose the stored data. This could mean that the PB session gets results it doesn't expect, and thus expose a way for PB mode to be detected. Review commit: https://reviewboard.mozilla.org/r/50765/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50765/
Attachment #8748450 -
Attachment description: MozReview Request: Bug 1268984 - Ensure GMPs are re-inserted in GMPServiceParent::mPlugins in the same order in ReAddOnGMPThread. r?jessup → MozReview Request: Bug 1268984 - Ensure GMPs are re-inserted in GMPServiceParent::mPlugins in the same order in ReAddOnGMPThread. r=jesup
Attachment #8749125 -
Flags: review?(gsquelart)
Attachment #8749126 -
Flags: review?(rjesup)
Assignee | ||
Comment 29•8 years ago
|
||
If you request a GMPParent with a nodeId, you should get any already running instances with the same nodeId in preference to cloning an existing GMP and assigning it the nodeId. This is ensures that EME GMP actors that are same-origin run in the same GMP instance. The GMP gtests are failing because of the cross-origin checks in GeckoMediaPluginServiceParent::SelectPluginForAPI(). The loop there selects the first GMPParent that can be used from the nodeId passed in. We previously assumed a GMPParent can be used from a nodeId if the GMPParent has the same nodeId, or if it has not loaded its process and it has no nodeId. The problem with assuming that, is if an in-use GMPParent with the target nodeId lies in the GeckoMediaPluginServiceParent::mPlugins list after a GMPParent with no nodeId, we'll end up using the first GMPParent (the one with no nodeId) rather than the one with the target nodeId. The solution is to change GeckoMediaPluginServiceParent::SelectPluginForAPI() so that effectively if we have a target nodeId, we'll select the first GMPParent that has the same nodeId, or we'll clone the first which supported all the requested capabilities/tags. This means when you request a GMPParent with a given nodeId, you'll get the one with the same nodeId (origin) by preference. Review commit: https://reviewboard.mozilla.org/r/50767/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50767/
Assignee | ||
Comment 30•8 years ago
|
||
Comment on attachment 8748450 [details] MozReview Request: Bug 1268984 - Ensure GMPs are re-inserted in GMPServiceParent::mPlugins in the same order in ReAddOnGMPThread. r=jesup Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50297/diff/2-3/
Comment 31•8 years ago
|
||
Comment on attachment 8749126 [details] MozReview Request: Bug 1268984 - Prefer to re-use a GMPParent with the requested nodeId rather than clone. r=jesup https://reviewboard.mozilla.org/r/50767/#review47481 ::: dom/media/gmp/GMPParent.cpp:82 (Diff revision 1) > + LOGD("GMPParent ctor id=%d", mPluginId); > } > > GMPParent::~GMPParent() > { > - LOGD("GMPParent dtor"); > + LOGD("GMPParent dtor id=%d", mPluginId); %u
Attachment #8749126 -
Flags: review?(rjesup) → review+
Comment on attachment 8749125 [details] MozReview Request: Bug 1268984 - Store GMPStorage on GMPServiceParent so that it persists inside the same PB session. r=gerald https://reviewboard.mozilla.org/r/50765/#review47489 Only 1 issue to fix, the rest are just suggestions. ::: dom/media/gmp/GMPMemoryStorage.cpp:82 (Diff revision 1) > + Record() : mIsOpen(false) {} > + nsTArray<uint8_t> mData; > + bool mIsOpen; You don't need to write the constructor if you add '= false' directly after the 'mIsOpen' declaration. ::: dom/media/gmp/GMPServiceParent.cpp:1313 (Diff revision 1) > + if (!mTempGMPStorage.Contains(aNodeId)) { > + RefPtr<GMPStorage> s(CreateGMPMemoryStorage()); > + mTempGMPStorage.Put(aNodeId, s); > + } You could 'return s.forget();' from this block, to avoid a redundant hashtable Get below. ::: dom/media/gmp/GMPStorage.h:9 (Diff revision 1) > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#ifndef GMPStorage_h_ > +#define GMPStorage_h_ > + > +#include "gmp-storage.h" To make this header self-sufficient, you should also #include AlreadyAddRefed.h, nsISupportsImpl.h, nsStringAPI.h, nsTArray.h. ::: dom/media/gmp/GMPStorage.h:19 (Diff revision 1) > + virtual bool IsOpen(const nsCString& aRecordName) = 0; > + virtual GMPErr Read(const nsCString& aRecordName, > + nsTArray<uint8_t>& aOutBytes) = 0; > + virtual GMPErr Write(const nsCString& aRecordName, > + const nsTArray<uint8_t>& aBytes) = 0; > + virtual GMPErr GetRecordNames(nsTArray<nsCString>& aOutRecordNames) = 0; IsOpen, Read, and GetRecordNames could probably be const. (If you agree, then you'll need to update GMPMemoryStorage and GMPDiskStorage as well.) ::: dom/media/gmp/GMPStorageParent.cpp:100 (Diff revision 1) > // and we resolve hash collisions by just adding 1 to the hash code. > // The format of records on disk is: > // 4 byte, uint32_t $recordNameLength, in little-endian byte order, > // record name (i.e. $recordNameLength bytes, no null terminator) > // record bytes (entire remainder of file) > class GMPDiskStorage : public GMPStorage { Since you've moved GMPMemoryStorage to its own file, why not emancipate GMPDiskStorage as well? ::: dom/media/gmp/GMPStorageParent.cpp:521 (Diff revision 1) > - MakeUnique<GMPDiskStorage>(mNodeId, mPlugin->GetPluginBaseName()); > + new GMPDiskStorage(mNodeId, mPlugin->GetPluginBaseName()); > if (NS_FAILED(storage->Init())) { > NS_WARNING("Failed to initialize on disk GMP storage"); > return NS_ERROR_FAILURE; > } > - mStorage = Move(storage); > + mStorage = storage; Don't be afraid of the Move. Embrace the Move! (As the block-local 'storage' RefPtr will expire at the next line anyway.)
Attachment #8749125 -
Flags: review?(gsquelart) → review+
Comment on attachment 8748450 [details] MozReview Request: Bug 1268984 - Ensure GMPs are re-inserted in GMPServiceParent::mPlugins in the same order in ReAddOnGMPThread. r=jesup https://reviewboard.mozilla.org/r/50297/#review47495 ::: dom/media/gmp/GMPServiceParent.cpp:1202 (Diff revision 3) > - // Don't re-add plugin if we're shutting down. Let the old plugin die. > + // We're not shutting down, so replace the old plugin in the list a > + // clone which is in a pristine state. Note: We place the plugin in 'replace ... with a clone' (missing 'with').
Attachment #8748450 -
Flags: review+
Assignee | ||
Comment 34•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=042f4a957aa0
Assignee | ||
Comment 35•8 years ago
|
||
(In reply to Gerald Squelart [:gerald] from comment #32) > Comment on attachment 8749125 [details] > MozReview Request: Bug 1268984 - Store GMPStorage on GMPServiceParent so > that it persists inside the same PB session. r?gerald > > ::: dom/media/gmp/GMPStorage.h:19 > (Diff revision 1) > > + virtual bool IsOpen(const nsCString& aRecordName) = 0; > > + virtual GMPErr Read(const nsCString& aRecordName, > > + nsTArray<uint8_t>& aOutBytes) = 0; > > + virtual GMPErr Write(const nsCString& aRecordName, > > + const nsTArray<uint8_t>& aBytes) = 0; > > + virtual GMPErr GetRecordNames(nsTArray<nsCString>& aOutRecordNames) = 0; > > IsOpen, Read, and GetRecordNames could probably be const. > (If you agree, then you'll need to update GMPMemoryStorage and > GMPDiskStorage as well.) I don't think we can do this for Read() for GMPDiskStorage, as its Read() implementation needs to call PR_Read which takes a non-const PRFileDesc.
Assignee | ||
Comment 36•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d6a8262b544
Assignee | ||
Comment 37•8 years ago
|
||
Comment on attachment 8749125 [details] MozReview Request: Bug 1268984 - Store GMPStorage on GMPServiceParent so that it persists inside the same PB session. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50765/diff/1-2/
Attachment #8749125 -
Attachment description: MozReview Request: Bug 1268984 - Store GMPStorage on GMPServiceParent so that it persists inside the same PB session. r?gerald → MozReview Request: Bug 1268984 - Store GMPStorage on GMPServiceParent so that it persists inside the same PB session. r=gerald
Attachment #8749126 -
Attachment description: MozReview Request: Bug 1268984 - Prefer to re-use a GMPParent with the requested nodeId rather than clone. r?jesup → MozReview Request: Bug 1268984 - Prefer to re-use a GMPParent with the requested nodeId rather than clone. r=jesup
Assignee | ||
Comment 38•8 years ago
|
||
Comment on attachment 8749126 [details] MozReview Request: Bug 1268984 - Prefer to re-use a GMPParent with the requested nodeId rather than clone. r=jesup Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50767/diff/1-2/
https://reviewboard.mozilla.org/r/50765/#review47659 r+ (again) after fixing this: ::: dom/media/gmp/GMPStorageParent.cpp:50 (Diff revisions 1 - 2) > if (persistent) { > - RefPtr<GMPDiskStorage> storage = > + mStorage = CreateGMPDiskStorage(mNodeId, mPlugin->GetPluginBaseName()); > - new GMPDiskStorage(mNodeId, mPlugin->GetPluginBaseName()); > - if (NS_FAILED(storage->Init())) { > - NS_WARNING("Failed to initialize on disk GMP storage"); > - return NS_ERROR_FAILURE; > - } > - mStorage = storage; > } else { > mStorage = mps->GetMemoryStorageFor(mNodeId); > } > > mShutdown = false; > return NS_OK; CreateGMPDiskStorage could return nullptr, in which case I think you need to set mShutdown to true (to avoid later nullptr dereferences), and return a failure code for good measure. I think it'd be best to handle that after the if(persistent) block, so it would manage GetMemoryStorageFor failures as well (even if the current implementation cannot fail).
Assignee | ||
Comment 40•8 years ago
|
||
Comment on attachment 8748450 [details] MozReview Request: Bug 1268984 - Ensure GMPs are re-inserted in GMPServiceParent::mPlugins in the same order in ReAddOnGMPThread. r=jesup Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50297/diff/3-4/
Assignee | ||
Comment 41•8 years ago
|
||
Comment on attachment 8749125 [details] MozReview Request: Bug 1268984 - Store GMPStorage on GMPServiceParent so that it persists inside the same PB session. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50765/diff/2-3/
Assignee | ||
Comment 42•8 years ago
|
||
Comment on attachment 8749126 [details] MozReview Request: Bug 1268984 - Prefer to re-use a GMPParent with the requested nodeId rather than clone. r=jesup Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50767/diff/2-3/
Assignee | ||
Comment 43•8 years ago
|
||
https://reviewboard.mozilla.org/r/50765/#review47659 > CreateGMPDiskStorage could return nullptr, in which case I think you need to set mShutdown to true (to avoid later nullptr dereferences), and return a failure code for good measure. > I think it'd be best to handle that after the if(persistent) block, so it would manage GetMemoryStorageFor failures as well (even if the current implementation cannot fail). I added an if(!mStorage){return NS_ERROR_FAILURE;} block after the if(persistent) block. mShutdown should be true already.
Assignee | ||
Comment 44•8 years ago
|
||
Comment on attachment 8749125 [details] MozReview Request: Bug 1268984 - Store GMPStorage on GMPServiceParent so that it persists inside the same PB session. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50765/diff/3-4/
Assignee | ||
Comment 45•8 years ago
|
||
Comment on attachment 8749126 [details] MozReview Request: Bug 1268984 - Prefer to re-use a GMPParent with the requested nodeId rather than clone. r=jesup Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50767/diff/3-4/
Comment 46•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/90a1b756b4ba https://hg.mozilla.org/integration/mozilla-inbound/rev/e5dd268f592d https://hg.mozilla.org/integration/mozilla-inbound/rev/4d03fa80f416
Comment 47•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/90a1b756b4ba https://hg.mozilla.org/mozilla-central/rev/e5dd268f592d https://hg.mozilla.org/mozilla-central/rev/4d03fa80f416
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 48•8 years ago
|
||
[Tracking Requested - why for this release]: This is a regression in 47 from Widevine bug 1222845. We will want to uplift this fix to Aurora 48 and Beta 47 after the fix is verified on Nightly 49.
tracking-firefox47:
--- → ?
Assignee | ||
Comment 49•8 years ago
|
||
Comment on attachment 8748450 [details] MozReview Request: Bug 1268984 - Ensure GMPs are re-inserted in GMPServiceParent::mPlugins in the same order in ReAddOnGMPThread. r=jesup Requesting uplift to 47 and 48 on all patches in this bug. Approval Request Comment [Feature/regressing bug #]: Regression from Widevine EME supporting, affecting WebRTC. [User impact if declined]: WebRTC when using H.264 will fail on the second and subsequent runs. [Describe test coverage new/current, TreeHerder]: We have lots of EME and WebRTC tests. [Risks and why]: Should be fairly low risk, just changing the order of things. [String/UUID change made/needed]: None.
Attachment #8748450 -
Flags: approval-mozilla-beta?
Attachment #8748450 -
Flags: approval-mozilla-aurora?
Hello Nils, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(drno)
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(drno)
Reporter | ||
Comment 51•8 years ago
|
||
Nightly 49.0a1 (2016-05-09) with widevine enabled no longer hangs for me on mozilla.github.io/webrtc-landing/pc_test.html
(In reply to Nils Ohlmeier [:drno] from comment #51) > Nightly 49.0a1 (2016-05-09) with widevine enabled no longer hangs for me on > mozilla.github.io/webrtc-landing/pc_test.html Awesome! Thank you for a prompt verification.
Status: RESOLVED → VERIFIED
Comment on attachment 8748450 [details] MozReview Request: Bug 1268984 - Ensure GMPs are re-inserted in GMPServiceParent::mPlugins in the same order in ReAddOnGMPThread. r=jesup Recent regression, fix was verified on Nightly, Aurora48+, Beta47+
Attachment #8748450 -
Flags: approval-mozilla-beta?
Attachment #8748450 -
Flags: approval-mozilla-beta+
Attachment #8748450 -
Flags: approval-mozilla-aurora?
Attachment #8748450 -
Flags: approval-mozilla-aurora+
Part 2 doesn't apply cleanly to beta. Don't know if part 3 does, didn't get that far.
Flags: needinfo?(cpearce)
Comment 55•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/eb0ba9144f45 https://hg.mozilla.org/releases/mozilla-aurora/rev/c74d913c3ac8 https://hg.mozilla.org/releases/mozilla-aurora/rev/bbbcc8fc51fa
Assignee | ||
Comment 56•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/254daaf1315f https://hg.mozilla.org/releases/mozilla-beta/rev/1182c6ecfe5e https://hg.mozilla.org/releases/mozilla-beta/rev/694986f3c986
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(cpearce)
Assignee | ||
Comment 57•8 years ago
|
||
Widevine (the regressor) is only in 47, not 46, so setting status-46=unaffected.
You need to log in
before you can comment on or make changes to this bug.
Description
•