WebRTC H264 video rendering issue

VERIFIED FIXED in Firefox 47

Status

()

P1
normal
Rank:
5
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: drno, Assigned: cpearce)

Tracking

(Blocks: 1 bug, {regression})

48 Branch
mozilla49
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 unaffected, firefox47+ fixed, firefox48 fixed, firefox49 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments)

(Reporter)

Description

2 years ago
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

2 years ago
backlog: --- → webrtc/webaudio+
Rank: 19
(Reporter)

Comment 1

2 years ago
Created attachment 8747287 [details]
rtp-video-mac.pcapng

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

2 years ago
Created attachment 8747288 [details]
rtp-video-linux.pcapng

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.
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

2 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

2 years ago
status-firefox47: --- → affected
status-firefox48: --- → affected
status-firefox49: --- → affected
(Reporter)

Updated

2 years ago
status-firefox46: --- → affected
(Reporter)

Comment 5

2 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

2 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.
status-firefox46: affected → ?
(Reporter)

Comment 7

2 years ago
Have we seen anything else then Mac suffering from this problem?
(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)
(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)
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
Update: the close-to-nightly mozconfig exhibits the grey video and/or freeze behavior just like official Nightly.
Created attachment 8748387 [details]
mozconfig for local build as close to nightly as I manage

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

2 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

2 years ago
Summary: Cisco Spark video rendering issue → WebRTC video rendering issue
(Reporter)

Comment 14

2 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

2 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

2 years ago
Created attachment 8748450 [details]
MozReview Request: Bug 1268984 - Ensure GMPs are re-inserted in GMPServiceParent::mPlugins in the same order in ReAddOnGMPThread. r=jesup

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/
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 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

2 years ago
Assignee: mfroman → cpearce
(Assignee)

Comment 19

2 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

2 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

2 years ago
backlog: webrtc/webaudio+ → ---
Component: WebRTC → Audio/Video: GMP
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).
Try failure in GTEST:
TEST-UNEXPECTED-FAIL | GeckoMediaPlugins.GMPCrossOrigin | Value of: mGMP && (mGMP->GetPluginId() == aGMP->GetPluginId()) == mShouldBeEqual
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: 1222845, 1265270
Keywords: regression

Updated

2 years ago
Rank: 19 → 5
(Assignee)

Comment 25

2 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.
See Also: → bug 1266336
(Assignee)

Comment 28

2 years ago
Created attachment 8749125 [details]
MozReview Request: Bug 1268984 - Store GMPStorage on GMPServiceParent so that it persists inside the same PB session. r=gerald

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

2 years ago
Created attachment 8749126 [details]
MozReview Request: Bug 1268984 - Prefer to re-use a GMPParent with the requested nodeId rather than clone. r=jesup

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

2 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 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 35

2 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 37

2 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

2 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

2 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

2 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

2 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

2 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

2 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

2 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 47

2 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
Last Resolved: 2 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
[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

2 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)

Updated

2 years ago
tracking-firefox47: ? → +
(Reporter)

Updated

2 years ago
status-firefox49: fixed → verified
Flags: needinfo?(drno)
(Reporter)

Comment 51

2 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)
(Assignee)

Updated

2 years ago
Flags: needinfo?(cpearce)
(Assignee)

Comment 57

2 years ago
Widevine (the regressor) is only in 47, not 46, so setting status-46=unaffected.
status-firefox46: ? → unaffected
You need to log in before you can comment on or make changes to this bug.