Closed Bug 1454630 Opened 6 years ago Closed 6 years ago

Widevine / Audio Decoding error with Shaka player

Categories

(Core :: Audio/Video: Playback, defect, P3)

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 61+ verified
firefox60 --- wontfix
firefox61 --- verified
firefox62 --- verified

People

(Reporter: jya, Assigned: jya)

References

(Blocks 2 open bugs,
URL
)

Details

Attachments

(4 files)

Bug 1454630 - P1. Simplify retrieval of current TrackInfo.
59 bytes, text/x-review-board-request
bryce
: review+
RyanVM
: approval-mozilla-beta+
RyanVM
: approval-mozilla-esr60+
Details
Bug 1454630 - P2. Use new PDMFactory whenever encryption type change.
59 bytes, text/x-review-board-request
bryce
: review+
cpearce
: review+
Details
Bug 1454630 - P3. Always recreate the PDMFactory when CDMProxy change.
59 bytes, text/x-review-board-request
bryce
: review+
Details
Bug 1454630 - P4. Only wait for CDMProxy if actually needed for the given decoder.
59 bytes, text/x-review-board-request
bryce
: review+
Details 
Spawned from bug 1447821.

Open https://shaka-player-demo.appspot.com/demo/#asset=https://dai.google.com/ondemand/dash/content/2474151/vid/tosr2-widevine/manifest.mpd;license=https://proxy.uat.widevine.com/proxy;lang=en-US;build=uncompiled

After the google home demo another video starts, after a few seconds an audio decoding error appears

 error: 3 (NS_ERROR_DOM_MEDIA_DECODE_ERR (0x806e0004) - virtual mozilla::MediaResult mozilla::FFmpegAudioDecoder<57>::DoDecode(mozilla::MediaRawData*, uint8_t*, int, bool*, mozilla::MediaDataDecoder::DecodedData&): FFmpeg audio error:-1094995529)
Rank: 25
Priority: -- → P3
Is there any chance if this issue can be pushed up the priority?

We couldn't find any workarounds to get this content played back successfully on Firefox.
This affects content that transitions from clear to encrypted initialization segments.  For example, in multi-Period DASH if the first Period is "fully" clear and the second has encrypted content, it will fail to play.

Note this also affects the video streams, but the first error reported is for audio.  Here is another reproduction that is created from two known good assets.  The first minute is from the 'Sintel 4k (multicodec)' asset on this site and the second minute is from 'Sintel 4k (multicodec, Widevine)'.  Both assets play fine by themselves.  Both the links below work fine on Chrome.

https://shaka-player-demo.appspot.com/demo/#asset=https://storage.googleapis.com/shaka-demo-assets/_bugs/firefox-clear-then-encrypted/dash.mpd;license=https://cwip-shaka-proxy.appspot.com/no_auth;lang=en-US;build=uncompiled;play

And here is the same source material but using MP4 only.  Firefox doesn't report any errors but the video shows bad decoding.

https://shaka-player-demo.appspot.com/demo/#asset=https://storage.googleapis.com/shaka-demo-assets/_bugs/firefox-clear-then-encrypted/mp4.mpd;license=https://cwip-shaka-proxy.appspot.com/no_auth;lang=en-US;build=uncompiled;play
Any chances you could create a shorter demonstration, one where you don't have to wait several minutes to experience the problem?

thank you
Flags: needinfo?(modmaker)
You could also seek to near the end after it starts.  But here is one where it fails around 22 seconds.  (10 seconds clear segments, 20 seconds encrypted that has a 10 second clear lead)

https://shaka-player-demo.appspot.com/demo/#asset=https://storage.googleapis.com/shaka-demo-assets/_bugs/firefox-clear-then-encrypted/short.mpd;license=https://cwip-shaka-proxy.appspot.com/no_auth;lang=en-US;play;build=uncompiled

Also some more specifics on what Shaka Player is doing here:  
- Attach a MediaKeys object to the <video>
- Append the clear init segment, then the clear segments for the first Period
- Append the encrypted init segment and append the encrypted media segments. 

The encrypted segments have 10 seconds of clear lead and it fails when trying to decode the first encrypted frame.  It should have the required keys.

I know there is an EME conformance test that verifies clear then encrypted segments.  But in that test all the data is appended at once.  Maybe Firefox is failing because we are waiting a bit before we append the encrypted segments?
Flags: needinfo?(modmaker)
Assignee: nobody → jyavenard
My gutfeel is that the special exception of the Android PDM that always re-use the decoder rather than creating a new one, will prevent this stream on an android device.... to be checked
[Tracking Requested - why for this release]: We don't play content mixing encrypted / clear
status-firefox60: --- → wontfix
status-firefox61: --- → affected
status-firefox62: --- → affected
status-firefox-esr60: --- → affected
tracking-firefox61: --- → ?
tracking-firefox-esr60: --- → ?
Really wish there was something we could do to improve our test coverage here.
status-firefox-esr52: --- → unaffected
tracking-firefox61: ? → +
tracking-firefox62: --- → +
tracking-firefox-esr60: ? → 61+
There is a test, however we fell into the cracks due to how the test is set up.

I'll open a bug in the wpt to add coverage for this particular problem. Or maybe our friends at Google can take over, we're pretty far stretched resource wise right now
Flags: needinfo?(modmaker)
I thought I had fixed the issue with Firefox on Android (forcing a new PDMFactory when the CDMProxy wasn't set for an encrypted stream), but always getting a decoder creation error when it needs to create a new one. Interestingly, Chrome for Android fails to decode those streams just the same.
Comment on attachment 8980998 [details]
Bug 1454630 - P2. Use new PDMFactory whenever encryption type change.

https://reviewboard.mozilla.org/r/247124/#review253194

Shouldn't we be able to write a test for this simply by appending an encrypted stream after a non-encrypted stream to a MediaSource (or vice versa)?
Attachment #8980998 - Flags: review?(cpearce) → review+
See Also: → 1464822
The reason we fail to create the audio decoder for the encrypted content is that when the CDMProxy is set, the PDMFactory will only attempt to use the AndroidDecoderModule for all codecs, but opus is disable for the Android PDM (the sample is encrypted opus). I'll spawned another bug
With this new version, stream #1 on description plays on Android (we incorrectly attempted to reuse the same decoder on android even if encryption changed).

Different approach, now use two distinct PDMFactory, one for encrypted content, one for cleared.
Comment on attachment 8980997 [details]
Bug 1454630 - P1. Simplify retrieval of current TrackInfo.

https://reviewboard.mozilla.org/r/247122/#review253552
Attachment #8980997 - Flags: review?(bvandyk) → review+
Comment on attachment 8980998 [details]
Bug 1454630 - P2. Use new PDMFactory whenever encryption type change.

https://reviewboard.mozilla.org/r/247124/#review253554
Attachment #8980998 - Flags: review?(bvandyk) → review+
Comment on attachment 8981142 [details]
Bug 1454630 - P3. Always recreate the PDMFactory when CDMProxy change.

https://reviewboard.mozilla.org/r/247238/#review253556
Attachment #8981142 - Flags: review?(bvandyk) → review+
Comment on attachment 8981143 [details]
Bug 1454630 - P4. Only wait for CDMProxy if actually needed for the given decoder.

https://reviewboard.mozilla.org/r/247240/#review253558
Attachment #8981143 - Flags: review?(bvandyk) → review+
Blocks: 1465073
Thanks for the quick reviews
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7c444cd068b2
P1. Simplify retrieval of current TrackInfo. r=bryce
https://hg.mozilla.org/integration/autoland/rev/16a0b5cfca59
P2. Use new PDMFactory whenever encryption type change. r=bryce,cpearce
https://hg.mozilla.org/integration/autoland/rev/250d768f4749
P3. Always recreate the PDMFactory when CDMProxy change. r=bryce
https://hg.mozilla.org/integration/autoland/rev/ed681c93dd5b
P4. Only wait for CDMProxy if actually needed for the given decoder. r=bryce
(In reply to Jean-Yves Avenard [:jya] from comment #11)
> There is a test, however we fell into the cracks due to how the test is set
> up.
> 
> I'll open a bug in the wpt to add coverage for this particular problem. Or
> maybe our friends at Google can take over, we're pretty far stretched
> resource wise right now

Sure, I'll write a wpt test for this.
Flags: needinfo?(modmaker)
Please request Beta approval on this when you feel that it has had sufficient bake time.
Flags: qe-verify+
Flags: needinfo?(jyavenard)
Flags: in-testsuite?
Comment on attachment 8980997 [details]
Bug 1454630 - P1. Simplify retrieval of current TrackInfo.


Approval Request Comment
[Feature/Bug causing the regression]: not a new regression.
[User impact if declined]: Encrypted content with clear segment may not play on desktop, will certainly not play on Android
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: manually verified.
[Needs manual test from QE? If yes, steps to reproduce]: steps to reproduce are provided in the description
[List of other uplifts needed for the feature/fix]: all patches of this ticket
[Is the change risky?]: no
[Why is the change risky/not risky?]: we maintain two different decoding states rather than attempting to re-use the same one. It's far less risky that way than what it used to be. For the most popular sites using EME (Netflix, Amazon Prime, etc) they only use fully encrypted content, as such it's the same as before.
[String changes made/needed]: no
Flags: needinfo?(jyavenard)
Attachment #8980997 - Flags: approval-mozilla-esr60?
Attachment #8980997 - Flags: approval-mozilla-beta?
Comment on attachment 8980997 [details]
Bug 1454630 - P1. Simplify retrieval of current TrackInfo.

Fix for broken EME playback on some major sites. Approved for 61.0b11.
Attachment #8980997 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:62.0) Gecko/20100101 Firefox/62.0
Build ID: 20180605141053

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180604143143

Verified as fixed on Nightly build 62.0a1 (2018-06-05) and Beta 61.0b11 on MacOS 10.13.5.
status-firefox61: fixed → verified
status-firefox62: fixed → verified
tracking-firefox61: + → ---
tracking-firefox62: + → ---
Flags: qe-verify+
At least Part 1 will need a rebased patch for ESR60 (unsure about the subsequent ones).
Flags: needinfo?(jyavenard)
the conflicts were due to bug 1448222 and bug 1456743 (P2). I've merged that last one in while at it.
Comment on attachment 8980997 [details]
Bug 1454630 - P1. Simplify retrieval of current TrackInfo.

Thanks for doing the rebase. Approved for ESR 60.1.
Attachment #8980997 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Flags: qe-verify+
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180621121604

Verified as fixed on ESR build 60.1.0esr on MacOS 10.13.5.
Status: RESOLVED → VERIFIED
status-firefox-esr60: fixed → verified
Flags: qe-verify+
Depends on: 1495735
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: