Closed Bug 1317628 Opened 8 years ago Closed 7 years ago

[EME][Fennec] Handle DummyKeyId in CDMCaps to unblock the decoding pipeline for Android Lollipop.

Categories

(Firefox for Android Graveyard :: Audio/Video, defect)

Product:
Firefox for Android Graveyard
Component:
Audio/Video
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: kikuo, Assigned: kikuo)

References

Details

Attachments

(2 files, 3 obsolete files)

Bug 1317628-[P1] Handling key-needed/key-added for encrypted media playback with Fennec in-process-decode mode on Android L.
58 bytes, text/x-review-board-request
jchen
: review+
Details
Bug 1317628-[P2] Handling key-needed/key-added/notifying for encrypted media playback with Fennec out-of-process-decode mode on Android L.
59 bytes, text/x-review-board-request
Details
Since there's no way to obtain the KeyId status after providing response to CDM on Android L. We need to handle DummyKeyId in CDMCaps and SamplesWaitForKey to unblock the wait-for-key mechanism.
Comment on attachment 8810929 [details] Bug 1317628-[P1] Handling key-needed/key-added for encrypted media playback with Fennec in-process-decode mode on Android L. https://reviewboard.mozilla.org/r/93198/#review93194 Since we're not able to obtain correct KeyId + Status from CDM on Lollipop, I've tried to implement some functions to achieve the goal and scoped the code in specific version.
Comment on attachment 8810929 [details] Bug 1317628-[P1] Handling key-needed/key-added for encrypted media playback with Fennec in-process-decode mode on Android L. https://reviewboard.mozilla.org/r/93200/#review93190 Since we're not able to obtain correct KeyId + Status from CDM on Lollipop, I've tried to implement some functions to achieve the goal and scoped the code in specific version.
What does Chromium do to report keystatuses on Lollipop? I'm not very keen on having dummy keyids. It's hacky. How does the CDM know that it's safe to proceed decoding (i.e. how does it know to send the kDummyKeyId key status changed to Gecko)? When we know it's safe to proceed with decoding, can we instead send a new IPC message from the CDM over to Gecko to tell Gecko that it should just assume that all keys are usable? I think in this case we can forgo reporting any status on the keys; we don't really know that status anyway. Or, we could change the demuxers to report all the keyIds in the media data (that would be present in the MP4/WebM container somewhere), and just mark those as usable when we receive the message.
(In reply to Chris Pearce (:cpearce) from comment #4) > What does Chromium do to report keystatuses on Lollipop? For Android Lollipop, the framework API "MediaDrm" doesn't support reporting "KeyStatus" information back to App [1], every time a response is updated into CDM successfully, it reports new-key-added [2][3]. That results to "MediaSourcePlayer::OnKeyAdded" [4] be called and then codec starts decoding pending buffers. From Android L platform's perspective, when MediaCodec reports a MediaCodecStatus '''MEDIA_CODEC_NO_KEY''', the playback will stop and wait until key added. It acts like MDSM entering/leaving a wait-for-key state. Apparently, we don't have that state. [1] https://cs.chromium.org/chromium/src/media/base/android/java/src/org/chromium/media/MediaDrmBridge.java?rcl=0&l=67 [2] http://androidxref.com/5.1.1_r6/xref/external/chromium_org/media/base/android/java/src/org/chromium/media/MediaDrmBridge.java#633 [3] http://androidxref.com/5.1.1_r6/xref/external/chromium_org/media/base/android/media_drm_bridge.cc#516 [4] http://androidxref.com/5.1.1_r6/xref/external/chromium_org/media/base/android/media_source_player.cc#756 [5] http://androidxref.com/5.1.1_r6/xref/external/chromium_org/media/base/android/media_source_player.cc#504 Started from Mashmallow, parts of MediaDrm bridging code were moved into Chromium project (removed from AOSP), and the implementation is much aligned to EME v1, i.e. reporting KeyStatus to HTML. But the machinery of entering wait-for-key state and resume-playback is similar. [6][7] [6] https://cs.chromium.org/chromium/src/media/base/android/media_codec_loop.cc?l=228 [7] https://cs.chromium.org/chromium/src/media/base/android/media_codec_loop.cc?l=72 It seems to me that Android performs a "EAFP" style in this decryption/decoding pipeline. For me, Gekco is like "LBYL" style when it comes to send buffer to decryptor & decoder. > > I'm not very keen on having dummy keyids. It's hacky. > > How does the CDM know that it's safe to proceed decoding (i.e. how does it > know to send the kDummyKeyId key status changed to Gecko)? It seems that this kDummyKeyId is only removed when the CDM is about to be destroyed. > > When we know it's safe to proceed with decoding, can we instead send a new > IPC message from the CDM over to Gecko to tell Gecko that it should just > assume that all keys are usable? > > I think in this case we can forgo reporting any status on the keys; we don't > really know that status anyway. > Sounds like a OnKeyAdd(sessionId) to me, and that's not different to what we have now "CDMCaps::SetKeyStatus(keyId, sessionId, status)" > Or, we could change the demuxers to report all the keyIds in the media data > (that would be present in the MP4/WebM container somewhere), and just mark > those as usable when we receive the message. It's like what has been done in the patch. If decoder is about to decrypt/decode a sample with KeyId_1 but CDMCaps is waiting for information from CDM, once CDMCaps is updated with a kDummyKeyId, the KeyId_1 is marked as usable, I did it one at a time sequentially to make the procedure smoother. MediaDrmCDMProxy::OnSessionMessage (License-Request) SessionId_1 ----> SessionId_2 --------------------------> SessionId_3 MediaDataDecoder::Input Sample/KeyId_1 -------> Sample/KeyId_2 ----------------------> Sample/KeyId_3 | | blocked blocked CDMCaps::SetKeyStatus ------------------> kDummyKeyId(sId_1) ----------> kDummyKeyId(sId_2) ------> kDummyKeyId(sId_3) | | unblock KeyId_1 unblock KeyId2 (if sId_X is one of the ongoing sessionIds, this is what |CDMCaps::AutoLock::NotifyKeyIdUsableIfWaitingForSessionId| does) ################################################################################## If (kDummyKeyId, sId, status) from CDM is received sooner before encrypted data being sent into decoder, this is usually the case that video starts with clear content for several seconds, then ... MediaDrmCDMProxy::OnSessionMessage (License-Request) SessionId_1 ---> SessionId_2 --------------------------> SessionId_3 CDMCaps::SetKeyStatus --> kDummyKeyId(sId_1) ----------> kDummyKeyId(sId_2) ---------> kDummyKeyId(sId_3) MediaDataDecoder::Input ---------> Sample/KeyId_1 -----------------> Sample/KeyId_2 -----------------------> Sample/KeyId_3 | If one of the ongoing sessionIds matches sId_1, then mark KeyId_1 to usable. (This is what |CDMCaps::AutoLock::UpdateFirstMetDummyKeyIdKeyStatus| does) ################################################################################## I'm thinking that maybe you want a simplified code. Do you mean that SamplesWaitingForKey should wait until CDMCaps receives certain amount (the number of current ongoing sessions) of kDummyKeyId status, then mark all waiting sample's keyIds usable ? Sorry to make your eyes bleed ...
But, I'm not quite sure that the necessity of supporting Widevine on Android L, as 1) The framework for EME is a bit out-of-dated, 2) the modification for gecko is quite hacky too.
(In reply to Kilik Kuo [:kikuo] from comment #5) > (In reply to Chris Pearce (:cpearce) from comment #4) > > What does Chromium do to report keystatuses on Lollipop? > > For Android Lollipop, the framework API "MediaDrm" doesn't support reporting > "KeyStatus" information back to App [1], every time a response is updated > into CDM successfully, it reports new-key-added [2][3]. That results to > "MediaSourcePlayer::OnKeyAdded" [4] be called and then codec starts decoding > pending buffers. > > From Android L platform's perspective, when MediaCodec reports a > MediaCodecStatus '''MEDIA_CODEC_NO_KEY''', the playback will stop and wait > until key added. It acts like MDSM entering/leaving a wait-for-key state. > Apparently, we don't have that state. Well, we do have waiting-for-key functionality. I understand it's implemented by using the "waiting for data" functionality of the MDSM. Part of the problem here is that your dummy key Id is actually a valid WebM keyId; WebM keyIds don't have to be 16bytes, they can be an arbitrary byte length. > > [1] > https://cs.chromium.org/chromium/src/media/base/android/java/src/org/ > chromium/media/MediaDrmBridge.java?rcl=0&l=67 > [2] > http://androidxref.com/5.1.1_r6/xref/external/chromium_org/media/base/ > android/java/src/org/chromium/media/MediaDrmBridge.java#633 > [3] > http://androidxref.com/5.1.1_r6/xref/external/chromium_org/media/base/ > android/media_drm_bridge.cc#516 > [4] > http://androidxref.com/5.1.1_r6/xref/external/chromium_org/media/base/ > android/media_source_player.cc#756 > [5] > http://androidxref.com/5.1.1_r6/xref/external/chromium_org/media/base/ > android/media_source_player.cc#504 > > Started from Mashmallow, parts of MediaDrm bridging code were moved into > Chromium project (removed from AOSP), and the implementation is much aligned > to EME v1, i.e. reporting KeyStatus to HTML. > But the machinery of entering wait-for-key state and resume-playback is > similar. [6][7] > > [6] > https://cs.chromium.org/chromium/src/media/base/android/media_codec_loop. > cc?l=228 > [7] > https://cs.chromium.org/chromium/src/media/base/android/media_codec_loop. > cc?l=72 > > It seems to me that Android performs a "EAFP" style in this > decryption/decoding pipeline. > For me, Gekco is like "LBYL" style when it comes to send buffer to decryptor > & decoder. This here seems to be the crux of the issue. The GMP decryption backend checks to see if the keys are usable before proceeding, but the Android platform (on older versions of Android) doesn't make that easy. I think your Android backend would be better to just try to decode and then detect when a sample has failed to decode due to lacking a key. And then hang onto the sample while waiting for a key, and then retry the decode after every MediaKeySession.update() resolves. So I think it makes sense for your Android backend to use a style more in line with how the platform behaves, at least on older Android. The easiest way to do this may be to have a different implementation of CDMCaps for Android L. > > > > I'm not very keen on having dummy keyids. It's hacky. > > > > How does the CDM know that it's safe to proceed decoding (i.e. how does it > > know to send the kDummyKeyId key status changed to Gecko)? > > It seems that this kDummyKeyId is only removed when the CDM is about to be > destroyed. > > > > > When we know it's safe to proceed with decoding, can we instead send a new > > IPC message from the CDM over to Gecko to tell Gecko that it should just > > assume that all keys are usable? > > > > I think in this case we can forgo reporting any status on the keys; we don't > > really know that status anyway. > > > > Sounds like a OnKeyAdd(sessionId) to me, and that's not different to what we > have now "CDMCaps::SetKeyStatus(keyId, sessionId, status)" > > > Or, we could change the demuxers to report all the keyIds in the media data > > (that would be present in the MP4/WebM container somewhere), and just mark > > those as usable when we receive the message. > > It's like what has been done in the patch. > > If decoder is about to decrypt/decode a sample with KeyId_1 but CDMCaps is > waiting for information from CDM, once CDMCaps is updated with a > kDummyKeyId, the KeyId_1 is marked as usable, I did it one at a time > sequentially to make the procedure smoother. > > MediaDrmCDMProxy::OnSessionMessage (License-Request) > SessionId_1 ----> SessionId_2 --------------------------> SessionId_3 > > MediaDataDecoder::Input > Sample/KeyId_1 -------> Sample/KeyId_2 ----------------------> Sample/KeyId_3 > | | > blocked blocked > > CDMCaps::SetKeyStatus > ------------------> kDummyKeyId(sId_1) ----------> kDummyKeyId(sId_2) > ------> kDummyKeyId(sId_3) > | | > unblock KeyId_1 unblock > KeyId2 > (if sId_X is one of the ongoing > sessionIds, this is what > > |CDMCaps::AutoLock::NotifyKeyIdUsableIfWaitingForSessionId| does) > > ############################################################################# > ##### > > If (kDummyKeyId, sId, status) from CDM is received sooner before encrypted > data being sent into decoder, this is usually the case that video starts > with clear content for several seconds, then ... > > MediaDrmCDMProxy::OnSessionMessage (License-Request) > SessionId_1 ---> SessionId_2 --------------------------> SessionId_3 > > CDMCaps::SetKeyStatus > --> kDummyKeyId(sId_1) ----------> kDummyKeyId(sId_2) ---------> > kDummyKeyId(sId_3) > > MediaDataDecoder::Input > ---------> Sample/KeyId_1 -----------------> Sample/KeyId_2 > -----------------------> Sample/KeyId_3 > | > If one of the ongoing sessionIds matches sId_1, then > mark KeyId_1 to usable. > (This is what > |CDMCaps::AutoLock::UpdateFirstMetDummyKeyIdKeyStatus| does) > > > ############################################################################# > ##### > > I'm thinking that maybe you want a simplified code. Yes, I want to minimize the hacks we need to support older platforms. > Do you mean that SamplesWaitingForKey should wait until CDMCaps receives > certain amount (the number of current ongoing sessions) of kDummyKeyId > status, then mark all waiting sample's keyIds usable ? It may make sense to have a different CDMCaps object only used on Android <= L which listens for MediaKeySession.update() promise resolves, and it has a flag to make it just report all keys as usable when any update() promise is resolved. You also need to be able to handle a decrypt&decode operation failing due to the lack of a key, and then you need to be able to recover from that, i.e. store decrypt&decode operations while they're pending, in case they fail, so you can re-try easily.
Thanks for your insights, Chris, it's very helpful to me. Now I have a clearer view to work on it. I'll remove the r? flag for now.
Attachment #8810929 - Flags: review?(cpearce)
I'm gonna separate these into severals. Make these code/modification restricted to ANDROID API version below 23.
Attachment #8810929 - Attachment is obsolete: true
Attachment #8817234 - Attachment is obsolete: true
Comment on attachment 8810929 [details] Bug 1317628-[P1] Handling key-needed/key-added for encrypted media playback with Fennec in-process-decode mode on Android L. https://reviewboard.mozilla.org/r/93198/#review98266 I was thinking over to minimize the impact to gecko around CDMCaps/SamplesWaitingForKey classes to handle the no-key cases on Android L or older version. Those were modifications regarding native/managed codes and eme related knowledge.
Attachment #8810929 - Flags: review?(nchen)
Attachment #8810929 - Flags: review?(cpearce)
Attachment #8817748 - Flags: review?(nchen)
Attachment #8817748 - Flags: review?(cpearce)
Comment on attachment 8810929 [details] Bug 1317628-[P1] Handling key-needed/key-added for encrypted media playback with Fennec in-process-decode mode on Android L. https://reviewboard.mozilla.org/r/93200/#review93348 ::: dom/media/platforms/android/MediaCodecDataDecoder.cpp:165 (Diff revision 3) > void > EMEVideoDataDecoder::Input(MediaRawData* aSample) > { > - if (mSamplesWaitingForKey->WaitIfKeyNotUsable(aSample)) { > + // For Android L or older, we cannot receive meaningful keyId from CDM. > + // So decoder's not going to wait for certain keyId until the key becomes usable. > + // Instead, decoder is decoding directly and responde to the error code. "responde" isn't a word. ::: dom/media/platforms/android/MediaCodecDataDecoder.cpp:294 (Diff revision 3) > void > EMEAudioDataDecoder::Input(MediaRawData* aSample) > { > - if (mSamplesWaitingForKey->WaitIfKeyNotUsable(aSample)) { > + // For Android L or older, we cannot receive meaningful keyId from CDM. > + // So decoder's not going to wait for certain keyId until the key becomes usable. > + // Instead, decoder is decoding directly and responde to the error code. "responde" isn't a word. It might be more accurate to say we retry later when the key may be usable. ::: dom/media/platforms/android/MediaCodecDataDecoder.cpp:543 (Diff revision 3) > > PodCopy(static_cast<uint8_t*>(directBuffer), aSample->Data(), aSample->Size()); > > CryptoInfo::LocalRef cryptoInfo = GetCryptoInfoFromSample(aSample); > if (cryptoInfo) { > - res = mDecoder->QueueSecureInputBuffer(inputIndex, 0, cryptoInfo, > + int status = MediaCodecUtil::QueueSecureInputBuffer(mDecoder, This is running the CDM in-process right? So there's no way that another message to the CDM can be queued up to run after the QueueSecureInputBuffer command is run but before the NotifyKeyNeeded() command is run? If that could happen, you could end up with the NotifyKeyNeeded message happeneing after the message that marks a session as usable happens, and so you'd forever assume that the session wasn't usable. ::: dom/media/platforms/android/MediaCodecDataDecoder.cpp:661 (Diff revision 3) > res = QueueEOS(); > BREAK_ON_DECODER_ERROR(); > } > } > > + // Both Audio/Video are blocked even only one of them needs keys. s/Both Audio\/Video are blocked even only one of them needs keys./Both Audio\/Video are blocked even if only one of them needs a key./ ::: dom/media/platforms/android/MediaCodecDataDecoder.cpp:665 (Diff revision 3) > > + // Both Audio/Video are blocked even only one of them needs keys. > + if (MediaDrmProxy::IsPreMarshmallow() && WaitForKeys()) { > + MonitorAutoLock lock(mMonitor); > + // Short timeout here to avoid busy looping. > + lock.Wait(10000); I'm worried by a 10 second timeout. Netflix, for example, has a 10 second timeout on playback start up. Is there some other way to get a notification when we can retry? If there's no other way to break out of this sleep, then can we at least make the timeout shorter, like 250ms? Polling 4 times per second seems safer than once every 10 seconds, assuming that the cost of polling is low. ::: mobile/android/base/java/org/mozilla/gecko/media/GeckoMediaDrmBridgeV21.java:91 (Diff revision 3) > return false; > } > > - private static void assertTrue(boolean condition) { > + protected static void assertTrue(boolean condition) { > if (DEBUG && !condition) { > throw new AssertionError("Expected condition to be true"); Looks like this function is using 2 space indent, but others in this file is using 4. You may as well fix that while you're here. ::: mobile/android/base/java/org/mozilla/gecko/media/GeckoMediaDrmBridgeV21.java:222 (Diff revision 3) > } > > try { > final byte [] keySetId = mDrm.provideKeyResponse(session.array(), response); > if (DEBUG) { > HashMap<String, String> infoMap = mDrm.queryKeyStatus(session.array()); What does queryKeyStatus return for the keys on pre-Marshmellow? Does it tell you whether the keys are usable? It's documented to exist in Android v18 and later, so it should be usable here, right? If it's telling you whether the keys are usable, why can't we use that to inform the key statuses? ::: mobile/android/base/java/org/mozilla/gecko/media/GeckoMediaDrmBridgeV21.java:312 (Diff revision 3) > + } > + > protected void HandleKeyStatusChangeByDummyKey(String sessionId) > { > SessionKeyInfo[] keyInfos = new SessionKeyInfo[1]; > keyInfos[0] = new SessionKeyInfo(DUMMY_KEY_ID, Do you still need to be sending the dummy key id changed events here?
Comment on attachment 8810929 [details] Bug 1317628-[P1] Handling key-needed/key-added for encrypted media playback with Fennec in-process-decode mode on Android L. https://reviewboard.mozilla.org/r/93200/#review93348 > What does queryKeyStatus return for the keys on pre-Marshmellow? Does it tell you whether the keys are usable? It's documented to exist in Android v18 and later, so it should be usable here, right? > > If it's telling you whether the keys are usable, why can't we use that to inform the key statuses? Paste the key/value set for your reference. InfoMap : key(PersistAllowed)/value(False) InfoMap : key(RenewAllowed)/value(True) InfoMap : key(LicenseType)/value(Streaming) InfoMap : key(PlaybackDurationRemaining)/value(2592000) InfoMap : key(PlayAllowed)/value(True) InfoMap : key(RenewalServerUrl)/value() InfoMap : key(LicenseDurationRemaining)/value(2591997) As you can see, there are no keyid information coming from this API. So we cannot know which keyid becomes usable in this session. and the key/value returned from this API is vendor defined ... so we just use this API to print out the information only for debug usage.
Comment on attachment 8810929 [details] Bug 1317628-[P1] Handling key-needed/key-added for encrypted media playback with Fennec in-process-decode mode on Android L. https://reviewboard.mozilla.org/r/93200/#review93348 Thanks for the reviews. > This is running the CDM in-process right? So there's no way that another message to the CDM can be queued up to run after the QueueSecureInputBuffer command is run but before the NotifyKeyNeeded() command is run? If that could happen, you could end up with the NotifyKeyNeeded message happeneing after the message that marks a session as usable happens, and so you'd forever assume that the session wasn't usable. Good catch, that could happen indeed! I'll come up a solution regarding this. > I'm worried by a 10 second timeout. Netflix, for example, has a 10 second timeout on playback start up. Is there some other way to get a notification when we can retry? > > If there's no other way to break out of this sleep, then can we at least make the timeout shorter, like 250ms? Polling 4 times per second seems safer than once every 10 seconds, assuming that the cost of polling is low. Oops, I thought that unit is based on micro-seconds because I saw the values of these two definitions ... #define PR_INTERVAL_MIN 1000UL #define PR_INTERVAL_MAX 100000UL My intention was to poll every 10 ms. I think I could make it wait for an interval returned from |NSPR_API(PRIntervalTime) PR_MillisecondsToInterval(PRUint32 milli)|. Anohter reason I tended not to wait infinitely here is because some other functions (i.e. ::Input, ::Flush, ::Drains ...) may acquire thsi lock. I don't think it's a good idea to wait infinitely. > Do you still need to be sending the dummy key id changed events here? Hmm... I not sure if there could be any usage of the keyStatuses from the MediaKeyStatusMap in javascript. I'll verify if it's no longer needed.
Comment on attachment 8810929 [details] Bug 1317628-[P1] Handling key-needed/key-added for encrypted media playback with Fennec in-process-decode mode on Android L. https://reviewboard.mozilla.org/r/93200/#review93348 > Good catch, that could happen indeed! I'll come up a solution regarding this. I've tried to avoid using lock to tie |QueueSequereInputBuffer| and |NotifyKeyNeeded| together. GeckoMediaDrmBridgeV2X and MediaCodecDataDecoder are in seperatly thread, I also tried not to provide a shared lock for both Classes ... especially through JNI. So I came up with this solution. The only failure case which I can think of is that if licenses are requested & provided more than once during the call to |GetLicensedSessionCount| and |MaybeNotifyKeyNeeded|. But I don't really sure if that(no-key exception during |QueueSecureInputBuffer| while multiple licenses were already provided into CDM) would happen. Does that make sense to you ? > Hmm... I not sure if there could be any usage of the keyStatuses from the MediaKeyStatusMap in javascript. > I'll verify if it's no longer needed. It's fine to remove it, removed !
Attachment #8817748 - Attachment is obsolete: true
Attachment #8817748 - Flags: review?(nchen)
Attachment #8817748 - Flags: review?(cpearce)
Comment on attachment 8818935 [details] Bug 1317628-[P2] Handling key-needed/key-added/notifying for encrypted media playback with Fennec out-of-process-decode mode on Android L. https://reviewboard.mozilla.org/r/98854/#review99106 Interfaces may be based on patch 1, I'd like to hold patch 2 review until all issues in patch 1 are discussed and fixed.
Comment on attachment 8810929 [details] Bug 1317628-[P1] Handling key-needed/key-added for encrypted media playback with Fennec in-process-decode mode on Android L. https://reviewboard.mozilla.org/r/93200/#review99150 ::: mobile/android/base/java/org/mozilla/gecko/media/MediaDrmProxy.java:51 (Diff revision 4) > private boolean mDestroyed; > private GeckoMediaDrm mImpl; > private String mDrmStubId; > > + @WrapForJNI > + public static boolean isPreMarshmallow() { Change to a field, i.e., private static final boolean isPreMarshmallow = AppConstants.Versions.preMarshmallow; ::: mobile/android/base/java/org/mozilla/gecko/util/MediaCodecUtil.java:6 (Diff revision 4) > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > + > +package org.mozilla.gecko.util; Put this in the media package.
Attachment #8810929 - Flags: review?(nchen) → review+
(In reply to Jim Chen [:jchen] [:darchons] from comment #21) > Comment on attachment 8810929 [details] > Bug 1317628-[P1] Handling key-needed/key-added for encrypted media playback > with Fennec in-process-decode mode on Android L. > > https://reviewboard.mozilla.org/r/93200/#review99150 > > ::: mobile/android/base/java/org/mozilla/gecko/media/MediaDrmProxy.java:51 > (Diff revision 4) > > private boolean mDestroyed; > > private GeckoMediaDrm mImpl; > > private String mDrmStubId; > > > > + @WrapForJNI > > + public static boolean isPreMarshmallow() { > > Change to a field, i.e., > > private static final boolean isPreMarshmallow = > AppConstants.Versions.preMarshmallow; Jim, thanks for the review, but the modification will lead "isPreMarshmallow" defined as "true" directly in FennecJNIWrapper.h after compilation. in FennecJNIWrapper.h """ static const bool IsPreMarshmallow = true; """ But AppConstants.Versions.preMarshmallow is still correctly set to false during run-time on Android 6.0+. Is there anything I misunderstood ?
Sorry, it should be non-final, i.e., > @WrapForJNI > private static boolean isPreMarshmallow = AppConstants.Versions.preMarshmallow;
Comment on attachment 8810929 [details] Bug 1317628-[P1] Handling key-needed/key-added for encrypted media playback with Fennec in-process-decode mode on Android L. https://reviewboard.mozilla.org/r/93200/#review99150 > Change to a field, i.e., > > private static final boolean isPreMarshmallow = AppConstants.Versions.preMarshmallow; Cool, that dispeled my doubt, I then looked it up and found here [1], this should be the place where intrigue happens : ) Great thanks !! [1] http://searchfox.org/mozilla-central/rev/a6a339991dbc650bab3aefe939cd0e5ac302274a/build/annotationProcessors/CodeGenerator.java#429
(In reply to Kilik Kuo [:kikuo] from comment #24) > Comment on attachment 8810929 [details] > Bug 1317628-[P1] Handling key-needed/key-added for encrypted media playback > with Fennec in-process-decode mode on Android L. > > https://reviewboard.mozilla.org/r/93200/#review99150 > > > Change to a field, i.e., > > > > private static final boolean isPreMarshmallow = AppConstants.Versions.preMarshmallow; > > Cool, that dispeled my doubt, I then looked it up and found here [1], this > should be the place where intrigue happens : ) Great thanks !! > > [1] > http://searchfox.org/mozilla-central/rev/ > a6a339991dbc650bab3aefe939cd0e5ac302274a/build/annotationProcessors/ > CodeGenerator.java#429 Correction, this was a reply to Comment 23.
Comment on attachment 8810929 [details] Bug 1317628-[P1] Handling key-needed/key-added for encrypted media playback with Fennec in-process-decode mode on Android L. https://reviewboard.mozilla.org/r/93198/#review100054 Fix issues and slightly code clean-up.
Attachment #8818935 - Attachment is obsolete: true
Comment on attachment 8810929 [details] Bug 1317628-[P1] Handling key-needed/key-added for encrypted media playback with Fennec in-process-decode mode on Android L. https://reviewboard.mozilla.org/r/93200/#review100052 ::: dom/media/platforms/android/MediaCodecDataDecoder.cpp:655 (Diff revisions 3 - 4) > bool isOutputDone = false; > AutoLocalJNIFrame frame(jni::GetEnvForThread(), 1); > MediaFormat::LocalRef outputFormat(frame.GetEnv()); > nsresult res = NS_OK; > > + const PRIntervalTime kWaitForKeyInterval = PR_MillisecondsToInterval(uint32_t(10)); Optional Nit: You could make that 10u instead of uint32_t(10) (I think). ::: dom/media/platforms/android/MediaCodecDataDecoder.cpp:533 (Diff revision 5) > + aOffset, > + aCryptoInfo, > + aPTS, > + aFlags); > + if (status == MediaCodecUtil::CODEC_STATUS_NO_KEY) { > + MediaDrmProxy::MaybeNotifyKeyNeeded(aDrmStubId, count); I'm wary of logic in one thread doing a get-then-set of state from another thread. Instead of trying to maintain a count of the number of sessions that have received updates and the number of sessions which failed to decrypt, can you instead assume all keys are usable until the decrypt fails, and once a decrypt fails, you wait until the next successful update sent to the CDM, whereupon you retry. Basically, set "waitForKey" to true where and when the queueSecureInputBuffer fails, and then set it to false on the next update. Does that make sense? You really want this to be as close to the lowest level of the stack, where queueSecureInputBuffer is called, so that there's no oppourtunity for messages to interleave. ::: mobile/android/base/java/org/mozilla/gecko/media/GeckoMediaDrmBridgeV21.java:334 (Diff revision 5) > mCallbacks.onSessionCreated(createSessionToken, promiseId, sessionId, request); > } > > protected void onSessionUpdated(int promiseId, byte[] sessionId) { > assertTrue(mCallbacks != null); > + notifyKeyAdded(new String(sessionId)); If a session gets multiple update messages before it gets a license, won't this cause mLicensedSessions to be incremented more times than the number of sessions there will be usable? ::: mobile/android/base/java/org/mozilla/gecko/media/GeckoMediaDrmBridgeV21.java:420 (Diff revision 5) > switch (event) { > case MediaDrm.EVENT_PROVISION_REQUIRED: > if (DEBUG) Log.d(LOGTAG, "MediaDrm.EVENT_PROVISION_REQUIRED"); > break; > case MediaDrm.EVENT_KEY_REQUIRED: > if (DEBUG) Log.d(LOGTAG, "MediaDrm.EVENT_KEY_REQUIRED"); Is there any useful information delivered in the EVENT_KEY_REQUIRED event?
Attachment #8810929 - Flags: review?(cpearce)
Comment on attachment 8810929 [details] Bug 1317628-[P1] Handling key-needed/key-added for encrypted media playback with Fennec in-process-decode mode on Android L. https://reviewboard.mozilla.org/r/93200/#review100052 > I'm wary of logic in one thread doing a get-then-set of state from another thread. > > Instead of trying to maintain a count of the number of sessions that have received updates and the number of sessions which failed to decrypt, can you instead assume all keys are usable until the decrypt fails, and once a decrypt fails, you wait until the next successful update sent to the CDM, whereupon you retry. > > Basically, set "waitForKey" to true where and when the queueSecureInputBuffer fails, and then set it to false on the next update. Does that make sense? > > You really want this to be as close to the lowest level of the stack, where queueSecureInputBuffer is called, so that there's no oppourtunity for messages to interleave. If I understand correctly, I'm doing the same idea here. "mLicensedSessions" is increamented by one when every thime a response is provided, so renaming it to "mLicensedRequests" should be much better, because it's actually counting number of the licensed requests. "mLicensedRequests" and "mOngoingSessions" are used (with synchronization protection between the thread of MediaCodec loop and the thread of GeckoMediaDrmBridge) to identify if there's any incompleted licesing requests when a decryption error occurs in MediaCodec. I don't want to acquire a lock in order to call |queueSecureInputBuffer| every time and just make sure the "waitForKey" being set to true without interleavd messages when a decryption error happens. Because this case happens rarely but we have to pay the cost of lock. I think you're suggesting that (please correct me if I'm wrong) ... place "waitForKey" in native code and set it to true for both A/V decoder to block the loop when decryption fails. Then, update "waitForKey" to false everytime when all licenses are provided (there's no ongoing license-request/renewal) in GeckoMediaDrmBridge. In that case, a timer in decoder may be activated to poll the status of CDM requests (maybe every several hundred ms.) in order to unblock the case where all licesnes are provided and setting "waitForKey=false" during the call between |queueSecureInputBuffer| and setting "waitForKey=true". Is that correct ? > If a session gets multiple update messages before it gets a license, won't this cause mLicensedSessions to be incremented more times than the number of sessions there will be usable? Does it really happen ？ I think, that might be the case where asking a license-request then being followed by a license-renewal or something ... But I never saw this case by now on Android, not sure if that would actually happen. It's not handled either in Android [1] Looking into the method |updateSession|, it's actually doing |provideKeyResponse|, so in fact, the idea of the variable "mLicensedSessions" should be renamed to "mLicensedRequest", sorry about the misleading. [1] http://androidxref.com/5.1.1_r6/xref/external/chromium_org/media/base/android/java/src/org/chromium/media/MediaDrmBridge.java > Is there any useful information delivered in the EVENT_KEY_REQUIRED event? Actaully nope, this event is defined in Android MediaDrm API, but I didn't see the event coming after we disable widevine specifici "privacy mode". According to the document found on the internet, ''' When privacyMode is enabled, the first call to getKeyRequest will return a service certificate request, which will be used to obscure the device identifying information in the subsequent license request. ''' For simple usage, we disabled it. So this event here is never fired now.
(In reply to Kilik Kuo [:kikuo] from comment #29) > Comment on attachment 8810929 [details] > Bug 1317628-[P1] Handling key-needed/key-added for encrypted media playback > with Fennec in-process-decode mode on Android L. > > https://reviewboard.mozilla.org/r/93200/#review100052 > > > I'm wary of logic in one thread doing a get-then-set of state from another thread. > > > > Instead of trying to maintain a count of the number of sessions that have received updates and the number of sessions which failed to decrypt, can you instead assume all keys are usable until the decrypt fails, and once a decrypt fails, you wait until the next successful update sent to the CDM, whereupon you retry. > > > > Basically, set "waitForKey" to true where and when the queueSecureInputBuffer fails, and then set it to false on the next update. Does that make sense? > > > > You really want this to be as close to the lowest level of the stack, where queueSecureInputBuffer is called, so that there's no oppourtunity for messages to interleave. > > If I understand correctly, I'm doing the same idea here. > "mLicensedSessions" is increamented by one when every thime a response is > provided, so renaming it to "mLicensedRequests" should be much better, > because it's actually counting number of the licensed requests. You're almost doing the same thing. The key difference is that you're maintaining a count across threads. And when you do that, you need to worry about keeping it synced up. We've had numerous bugs that fail due to a design like this. You're also much more likely to hit hard-to-repro edge cases when you scale up to millions of users. > I don't want to acquire a lock in order to call |queueSecureInputBuffer| > every time and just make sure the "waitForKey" being set to true without > interleavd messages when a decryption error happens. Because this case > happens rarely but we have to pay the cost of lock. Chances are, locks only hurt you when they're in a tight loop, or when you end up spending time waiting on the lock. > I think you're suggesting that (please correct me if I'm wrong) ... place > "waitForKey" in native code and set it to true for both A/V decoder to block > the loop when decryption fails. Then, update "waitForKey" to false > everytime when all licenses are provided (there's no ongoing > license-request/renewal) in GeckoMediaDrmBridge. > > In that case, a timer in decoder may be activated to poll the status of CDM > requests (maybe every several hundred ms.) in order to unblock the case > where all licesnes are provided and setting "waitForKey=false" during the > call between |queueSecureInputBuffer| and setting "waitForKey=true". > Is that correct ? Basically, yes. I don't know if "in native" code is the best place, but yes, that's the basic idea. > > If a session gets multiple update messages before it gets a license, won't this cause mLicensedSessions to be incremented more times than the number of sessions there will be usable? > > Does it really happen ？ I think, that might be the case where asking a > license-request then being followed by a license-renewal or something ... > But I never saw this case by now on Android, not sure if that would actually > happen. It's not handled either in Android [1] > > Looking into the method |updateSession|, it's actually doing > |provideKeyResponse|, so in fact, the idea of the variable > "mLicensedSessions" should be renamed to "mLicensedRequest", sorry about the > misleading. I'm mostly worried about a remove interleaving.
Comment on attachment 8810929 [details] Bug 1317628-[P1] Handling key-needed/key-added for encrypted media playback with Fennec in-process-decode mode on Android L. https://reviewboard.mozilla.org/r/93200/#review100052 > If I understand correctly, I'm doing the same idea here. > "mLicensedSessions" is increamented by one when every thime a response is provided, so renaming it to "mLicensedRequests" should be much better, because it's actually counting number of the licensed requests. > > "mLicensedRequests" and "mOngoingSessions" are used (with synchronization protection between the thread of MediaCodec loop and the thread of GeckoMediaDrmBridge) to identify if there's any incompleted licesing requests when a decryption error occurs in MediaCodec. > > I don't want to acquire a lock in order to call |queueSecureInputBuffer| every time and just make sure the "waitForKey" being set to true without interleavd messages when a decryption error happens. Because this case happens rarely but we have to pay the cost of lock. > > I think you're suggesting that (please correct me if I'm wrong) ... place "waitForKey" in native code and set it to true for both A/V decoder to block the loop when decryption fails. Then, update "waitForKey" to false everytime when all licenses are provided (there's no ongoing license-request/renewal) in GeckoMediaDrmBridge. > > In that case, a timer in decoder may be activated to poll the status of CDM requests (maybe every several hundred ms.) in order to unblock the case where all licesnes are provided and setting "waitForKey=false" during the call between |queueSecureInputBuffer| and setting "waitForKey=true". > Is that correct ? Refactor the code again to avoid using those counting variables, and tied |QueueSecureInputBuffer| and |NotifyKeyNeeded| with a lock. So that it won't be interleaved by message updates. > Does it really happen ？ I think, that might be the case where asking a license-request then being followed by a license-renewal or something ... But I never saw this case by now on Android, not sure if that would actually happen. It's not handled either in Android [1] > > Looking into the method |updateSession|, it's actually doing |provideKeyResponse|, so in fact, the idea of the variable "mLicensedSessions" should be renamed to "mLicensedRequest", sorry about the misleading. > > [1] http://androidxref.com/5.1.1_r6/xref/external/chromium_org/media/base/android/java/src/org/chromium/media/MediaDrmBridge.java License{Request,Renewal,Release) are defined in Android API V23. It's not possible to handle multiple updates for one single session in gecko side, we're only able to bypass these responses to CDM. Since there's no counting variables in the new patch, should be fixed.
Comment on attachment 8810929 [details] Bug 1317628-[P1] Handling key-needed/key-added for encrypted media playback with Fennec in-process-decode mode on Android L. Comparing to current EME spec, the framework's API on Android L is a bit out-of-date. To support it may result to more potential bugs and we're not actually being asked for it now. So, I'll attach the solutions for both in-process & out-of-process decode for reference and move on to other tasks. Also, thanks to Chris's & Jim's time and feedback.
Attachment #8810929 - Flags: review?(cpearce)
Comment on attachment 8821140 [details] Bug 1317628-[P2] Handling key-needed/key-added/notifying for encrypted media playback with Fennec out-of-process-decode mode on Android L. https://reviewboard.mozilla.org/r/100510/#review101032 P2 now is much simpler than previous version.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: