1267141 - (GMP_Clearkey_on_Fennec) Enable GMP Clearkey support on Fennec.

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: Audio/Video, task, P2)

Description

[James Cheng[:JamesCheng]]

The final goal is to enable EME supporting on clearkey, widevine, etc.

First, we would like to enable 
1. EME API in this bug.
2. Enable Clearkey in this bug.

Comment 1

[James Cheng[:JamesCheng]]

Attachment: Part1 - Enable-EME-API-on-fennec.patch

Enable EME API on fennec platform.

Comment 2

[James Cheng[:JamesCheng]]

Hi spohl,

We would like to enable clearkey solution on Fennec.

The gmp-clearkey module will be built and located in |obj-arm-linux-androideabi/dist/bin/gmp-clearkey/0.1|.

This folder contains two files which are |clearkey.info| and |libclearkey.so| and we want to put those two files into Fennec apk and automatically put it into apk's storage.

GMPProvider.jsm [1] will try to add the plugin folder but the |GreDir| we get on Fennec will be |data/data/org.mozilla.fennec_xxx/”|.

Currently for testing, I manually |adb push| these two files into that folder to let GMPService successfully load the library.

Could you please give me some advise that how to put these two files into data/data/org.mozilla.fennec_xxx/gmp-clearkey/0.1/ in elegant way? Or any other better way for solving this problem.

Thank you very much.

[1] https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/toolkit/mozapps/extensions/internal/GMPProvider.jsm#609

Comment 3

[Stephen A Pohl [:spohl]]

Are you asking how to package these additional files into the apk at build/package time? I'm not very familiar with Fennec and how it's built/packaged, but looking at the history of toolkit/mozapps/installer/upload-files-APK.mk it would seem like nalexander and/or glandium might be able to help here.

Comment 4

[James Cheng[:JamesCheng]]

Hello nalexander,

Based on comment 2, our goal is to make

|obj-arm-linux-androideabi/dist/bin/gmp-clearkey/0.1/clearkey.info|
|obj-arm-linux-androideabi/dist/bin/gmp-clearkey/0.1/libclearkey.so|

be put into

|data/data/org.mozilla.fennec_xxx/gmp-clearkey/0.1/clearkey.info|
|data/data/org.mozilla.fennec_xxx/gmp-clearkey/0.1/libclearkey.so|

and let GMP Service load the library when fennec is launching.

I think it is just like whom puts these librarys into this folder.
|/data/data/org.mozilla.fennec_xxx/lib/libmozglue.so and libplugin-container.so|

Could you please give me some suggestion how to do this or forward me to the right person :)

Thank you very much.

Comment 5

[Nick Alexander :nalexander [he/him]]

(In reply to James Cheng[:JamesCheng] from comment #4)
> Hello nalexander,
> 
> Based on comment 2, our goal is to make
> 
> |obj-arm-linux-androideabi/dist/bin/gmp-clearkey/0.1/clearkey.info|
> |obj-arm-linux-androideabi/dist/bin/gmp-clearkey/0.1/libclearkey.so|
> 
> be put into
> 
> |data/data/org.mozilla.fennec_xxx/gmp-clearkey/0.1/clearkey.info|
> |data/data/org.mozilla.fennec_xxx/gmp-clearkey/0.1/libclearkey.so|

I think these will be loaded with Fennec's linker (like libxul.so) and not the Android linker (like libmozglue.so).  That means it'll be much better if you pack them into assets/, like libxul.so.  If that's the case, you'll want to handle them the same.  They're included using this flag:

https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/action/package_fennec_apk.py#106

those are actually coming from:

https://dxr.mozilla.org/mozilla-central/source/mobile/android/installer/package-manifest.in#29

So you can probably just add

; media
#ifdef MOZ_EME
@RESPATH@/gmp-clearkey/0.1/@DLL_PREFIX@clearkey@DLL_SUFFIX@
@RESPATH@/gmp-clearkey/0.1/clearkey.info
#endif

around there.  (There may be issues with paths getting chopped, etc.  If you need, add a new section with a new destdir.)

> and let GMP Service load the library when fennec is launching.

It's not clear exactly how this works.  In general, we read from the assets/ folder of the APK, memory mapped as a ZIP file.  You can't really know where the system unpacks libraries to; reading from /data/data is not always easy.

Let me know how that works, and what your linker requirements really are.  glandium is very knowledgable here too.

Comment 6

[James Cheng[:JamesCheng]]

Hi nalexander and glandium,

I explained more detailed as below,

Since GMPProvider.js[1] will add gmp plugin folder by greDir.path,

the greDir.path will be |data/data/org.mozilla.fennec_xxx/| on fennec platform.

And GMPLoader[2] will try to load the library(dlopen) by this folder that GMPProvider added.

Therefore, I want to find a way to put those .so and .info files into |data/data/org.mozilla.fennec_xxx/| to fulfill the current GMPProvider search path.

Maybe there is some better way to put the library to the better location and we can modify the GMPProvider.js to change the search path.

Please give me some advise for this.

Thank you very much.



[1] https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/toolkit/mozapps/extensions/internal/GMPProvider.jsm#609

[2] https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/dom/media/gmp/GMPLoader.cpp#327

Comment 7

[James Cheng[:JamesCheng]]

Hello nalexander,
I found that when I ./mach install the apk to the real device and "not" launch Fennec.
I saw that libmozglue.so, libplugin-container-pie.so, libplugin-container.so these three files will automatically store into |/data/data/org.mozilla.fennec_xxx/lib/|.

I try to add my data file to package-manifest.in
https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/mobile/android/installer/package-manifest.in#76

the file only appear in apk's/lib but not appear in the data/data/org.mozilla.fennec_xxx/lib folder.

Do you know where the magic happened? I cannot find that...

Thanks.

Comment 8

[Blake Wu [:bwu][:blakewu]]

I think Fred may know it.

Comment 9

[[e-mail]]

You should needinfo someone works on fannec, because it might be related to their build system(gradle)

Comment 10

[Mike Hommey [:glandium]]

If the module is shipped in fennec, it is better if it lives in assets/, and if it's dlopen()ed with the special path /data/.../fennec....apk!/assets/filename.so, which will load it from the apk directly, like we do for e.g. OMX plugins. It is possible to get that apk!/assets/ path for libxul.so, to derive paths from, from js (see toolkit/components/lz4/lz4_internal.js) and C++ (see dom/media/android/AndroidMediaPluginHost.cpp)

Comment 11

[Nick Alexander :nalexander [he/him]]

I echo what glandium says.  Best in /assets, following the existing conventions.

Comment 12

[Kilik Kuo [:kikuo] (inactive)]

Hi James,

I'd like to continue working on this. May I take this bug ?

Comment 13

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Preload clearkey.so for GMPChild process to avoid any nsFile operation in GMPService.

The original plan is to leverage Android MediaDrm & MediaCrypto API with AOSP libdrmclearkeyplugin.so. James and I actually made it work ! But there are some drawbacks.

1. MediaCrypto API is added in 16 and MediaDrm API is in 18 with the capability of EME 0.1b.
2. Notification for key status changing on Android is supported in API level 23.
3. InitDataType "keyids" is not supported by AOSP libdrmclearkeyplugin.so.
4. Chromium has clearkey implementation inside itself, not using libdrmclearkeyplugin.so.

Based on these facts, we decided to enable EME clearkey on Fennec by our own clearkey implementation via GMP.

Hacks in this patch ...
1. Compressed libclearkey.so & clearkey.info are placed inside "/data/app/APK_NAME.apk!/assests/ANDROID_CPU_ARCH/gmp-clearkey/0.1", GMP checks the existence of the plugin and its .info via util-functions like [1][2], but I can't find a way to make it work against the path above. So I copied the necessary files out of package to "/data/data/APK_NAME/gmp-clearkey/0.1" for the capability examination.

2. It seems that compressed libraries can only be loaded inside package via specific Elf loaders. I'm not sure if there's a way to decompress/load these libraries outside the package.

Hi glandium, I would appreciate if you have any advice for me.

[1] https://dxr.mozilla.org/mozilla-central/rev/39dffbba764210b25bfc1e749b4f16db77fa0d46/dom/media/gmp/GMPChild.cpp#323
[2] https://dxr.mozilla.org/mozilla-central/rev/39dffbba764210b25bfc1e749b4f16db77fa0d46/dom/media/gmp/GMPUtils.cpp#69

Comment 14

[James Cheng[:JamesCheng]]

Please continue to work on this. Thanks

Comment 15

[Mike Hommey [:glandium]]

GMPChild/GNPUtils should be able to find the files through jar: urls (just prepend jar: to the /data/app/APK_NAME.apk!/assets... path). That obviously isn't a nsIFile, but there are XPCOM APIs to read from there (and it's a bummer that we don't have better APIs for that... bug the XPCOM module owner, perhaps).

Comment 16

[Kilik Kuo [:kikuo] (inactive)]

Thanks for the tips, glandium.

I've another thought instead of empowering XPCOM APIs support on non-nsIFile, that is, can we make CustomElf be able to load a compressed library which is out of .apk! ?
For other GMP cases (e.g. gmp-gmpopenh264), the decompressed library and .info file are downloaded and placed under 'ProfD' directory.  I'd like to hack into |ElfLoader::GetMappableFromPath| [1] to see if it's doable and to understand more about it, here looks like magic to me.

I'll also file a bug against XPCOM APIs capability once I finish the hack. Thanks again !!

[1] https://dxr.mozilla.org/mozilla-central/rev/9b428173a0889f5a25e7a6e855f2d1726207a723/mozglue/linker/ElfLoader.cpp#423

Comment 17

[Mike Hommey [:glandium]]

(In reply to Kilik Kuo [:kikuo] from comment #16)
> Thanks for the tips, glandium.
> 
> I've another thought instead of empowering XPCOM APIs support on
> non-nsIFile, that is, can we make CustomElf be able to load a compressed
> library which is out of .apk! ?

That's exactly what it does!

Comment 18

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Mike Hommey [:glandium] from comment #17)
> (In reply to Kilik Kuo [:kikuo] from comment #16)
> > Thanks for the tips, glandium.
> > 
> > I've another thought instead of empowering XPCOM APIs support on
> > non-nsIFile, that is, can we make CustomElf be able to load a compressed
> > library which is out of .apk! ?
> 
> That's exactly what it does!

Sorry, I might not describe clearly, the scenario is like following,

1. I extracted "/data/app/APK_NAME.apk!/SOMEWHERE/libA.so" and stored to "/data/data/APK_NAME/SOMEWHERE/libB.so", like [1].
2. Then I expected "libB.so" to be loaded as a MappbleExtractFile, but in fact it's loaded as a MappbleFile and system failed to load it, because there's no '!' in its path and even it's not in correct zip format (no end of central directory).

[1] https://dxr.mozilla.org/mozilla-central/rev/95ffbc4ff63584631c408e8d9912961fcf68bb09/mobile/android/base/java/org/mozilla/gecko/mozglue/GeckoLoader.java#250-337

So I'm wondering that should I decompress the stream of lib first by [2] while extracting it ? or
Is it possible to load a library which is not inflated but located outside the APK. (no '!' in its path) ?

[2] https://dxr.mozilla.org/mozilla-central/rev/95ffbc4ff63584631c408e8d9912961fcf68bb09/mobile/android/base/java/org/mozilla/gecko/mozglue/NativeZip.java#73-85

Comment 19

[Mike Hommey [:glandium]]

Why don't you just load it from the apk?

Comment 20

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Mike Hommey [:glandium] from comment #19)
> Why don't you just load it from the apk?

I'm not familiar with Fennec system, so I was trying to find out any possible ways for my intention which is making GMPService be able to read text files and to load libraries from APK.

My first solution is ... to extract files needed to data dir.
For text - no effort is needed
For lib  - I found the .so was not loadable as I mentioned in Comment 18.

My second solution is ... to keep everything in apk.
For text - either new XPCOM APIs which support operations on non-nsIFile path or a new specific function which handles the path and content by using modules in "modules/libjar".
For lib  - no effort is needed, the custom linker does a great job.

I suppose the 2nd solution is more reasonable and just want to figure out if the 1st sol. is also doable. Because I've spent some time tracing these codes and wanna achieve something on it, then I'll start implementing the 2nd solution.

Comment 21

[Mike Hommey [:glandium]]

For the .so to be loadable outside the apk, one of the following must be done:
- those .so files must not be szip'ed in the first place
- those .so files must be unszip'ed when extracting them
- the linker must be modified to support szip files outside the apk

> either new XPCOM APIs which support operations on non-nsIFile path or a new specific function which handles the path and content by using modules in "modules/libjar".

There are other possibilities, like using NS_NewChannel (https://dxr.mozilla.org/mozilla-central/source/extensions/pref/autoconfig/src/nsReadConfig.cpp#250 is close-ish to what you'd want (although, don't use PR_Malloc): create a nsIURI for jar:file:///data/app/apkname.apk!/path/to/your/file, create a nsIChannel for that URL, open it into a nsIInputStream, from which you can get data).

(And you could make the GMPInfoFileParser use the same code path with a file:// url for the normal path, and remove some of the NSPR usage it does ;) )

Comment 22

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Mike Hommey [:glandium] from comment #21)
> For the .so to be loadable outside the apk, one of the following must be
> done:
> - those .so files must not be szip'ed in the first place
> - those .so files must be unszip'ed when extracting them
> - the linker must be modified to support szip files outside the apk

Thanks glandium !! The summary indeed clears my doubts.
> 
> > either new XPCOM APIs which support operations on non-nsIFile path or a new specific function which handles the path and content by using modules in "modules/libjar".
> 
> There are other possibilities, like using NS_NewChannel
> (https://dxr.mozilla.org/mozilla-central/source/extensions/pref/autoconfig/
> src/nsReadConfig.cpp#250 is close-ish to what you'd want (although, don't
> use PR_Malloc): create a nsIURI for
> jar:file:///data/app/apkname.apk!/path/to/your/file, create a nsIChannel for
> that URL, open it into a nsIInputStream, from which you can get data).
> 
> (And you could make the GMPInfoFileParser use the same code path with a
> file:// url for the normal path, and remove some of the NSPR usage it does
> ;) )

Sounds great, I'll try it out :)  Thanks !

Comment 23

[Kilik Kuo [:kikuo] (inactive)]

Now I can read the content of clearkey.info inside APK as byte string without nsIFile interface. But the operation of {NS_NewURI,NS_NewChannel} requires to be done in main thread.  |NetUtil.asyncFetch| is able to be used to read the file in.

I'd like to provide a new API in mozIGeckoMediaPluginChromeService.idl which passes the jar DIR_PATH("jar:/data/app/APK_NAME!/assets/ABI/GMPPlugin_Dir") and the clearkey.info content(as byte string) into GMPService. e.g. |GeckoMediaPluginServiceParent::AddPluginJarDirectoryAndInfo(nsAString aAPK_DIR, nsAString aInfoContent)|

Also, new APIs, i.e. GMPParent::Init(nsAString aAPK_DIR) & GMPInfoFileParser::Init(nsAString aAPK_DIR) are needed to handle non-nsIFile path for this case.  Basically, all operations to GMPParent::mDirectory should be taken care of, and to ensure we can launch GMPChildProcess by this GMPPlugin_Dir which indicates to a entry in APK.

Now I'm writing a WIP-POC patch as an alternative solution of Attachment 8767487 [details] [diff].

Comment 24

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Handle URI on GMPProvider.jsm & handle nsString path in GMP{Service,Parent,Child} for clearkey only.

Hi Chris,

Per Comment 13, Comment 21, I enabled built-in clearkey by 
1. Obtain plugin directory & plugin info via URI/Channel in GMPProvider.jsm.
2. Operate on path string (not a nsIFile) in GMPParent/GMPChild.

I'd appreciate if you could give me some advices regarding to this 2nd solution or the 1st one [1].

Also, I am thinking to encapsulate the operations for paths into a unified class, e.g. "class GMPUniPath", and provide APIs i.e. Get/Set/Parse/Leaf/ParentLeaf. In that case, GMPParent::mDirectory and the new GMPParent::mPluginAPKDir won't exist.

Though I was thinking to use nsIURI to unify all operations, but it seems to be able to used in main thread.  Can it be used in other threads ?

[1] attachment 8767487 [details] [diff] [review]

Comment 25

[Chris Pearce [:cpearce (Not reading bugmail)]]

Can you do one of the following:

1. unzip gmp-clearkey to somewhere in the profile dir in when starting up GMPProvider if it's not yet been unzipped on startup.
2. unzip gmp-clearkey to somewhere in the profile dir when ClearKey is used for the first time. Possibly as part of resolving the MediaKeySystemAccess.createMediaKeys() promise.

Then you could use the existing code as-is, right? Wouldn't that be simpler?

I'd prefer option 2.

I'd prefer to avoid having a special path for gmp-clearkey on Android.

Comment 26

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8770840 [details] [diff] [review]
Handle URI on GMPProvider.jsm & handle nsString path in GMP{Service,Parent,Child} for clearkey only.

Clearing the f?. Please re-request feedback if my suggestions aren't practical.

Comment 27

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Chris Pearce (:cpearce) from comment #25)
> Can you do one of the following:
> 
> 1. unzip gmp-clearkey to somewhere in the profile dir in when starting up
> GMPProvider if it's not yet been unzipped on startup.
> 2. unzip gmp-clearkey to somewhere in the profile dir when ClearKey is used
> for the first time. Possibly as part of resolving the
> MediaKeySystemAccess.createMediaKeys() promise.
> 
> Then you could use the existing code as-is, right? Wouldn't that be simpler?
> 
> I'd prefer option 2.
> 
> I'd prefer to avoid having a special path for gmp-clearkey on Android.

Thanks for the advices, I attempted to copy clearkey.info & libclearkey.so out of APK in the 1st-patch [1], but zipped-.so is extracted, then I started to explore other solutions because of Comment 20.
Gecko has its own clearkey implementation and if it's packed into APK, I thought it would be storage-efficient and more secure by loading it directly inside APK without extra copy.

Considering to decompress the zipped stream, maybe I should use java class "ZipInputStream" as the container of "FileInputStream". (not familiar with Java:()
Also, I'll give this a try [2].

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8767487&action=diff#a/mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java_sec2
[2] https://dxr.mozilla.org/mozilla-central/rev/95ffbc4ff63584631c408e8d9912961fcf68bb09/mobile/android/base/java/org/mozilla/gecko/mozglue/NativeZip.java#73-85

Comment 28

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Kilik Kuo [:kikuo] from comment #27)
> (In reply to Chris Pearce (:cpearce) from comment #25)
> > Can you do one of the following:
> > 
> > 1. unzip gmp-clearkey to somewhere in the profile dir in when starting up
> > GMPProvider if it's not yet been unzipped on startup.
> > 2. unzip gmp-clearkey to somewhere in the profile dir when ClearKey is used
> > for the first time. Possibly as part of resolving the
> > MediaKeySystemAccess.createMediaKeys() promise.
> > 
> > Then you could use the existing code as-is, right? Wouldn't that be simpler?
> > 
> > I'd prefer option 2.
> > 
> > I'd prefer to avoid having a special path for gmp-clearkey on Android.
> 
> Considering to decompress the zipped stream, maybe I should use java class
> "ZipInputStream" as the container of "FileInputStream". (not familiar with
> Java:()
> Also, I'll give this a try [2].
> 
> [1]
> https://bugzilla.mozilla.org/attachment.cgi?id=8767487&action=diff#a/mobile/
> android/base/java/org/mozilla/gecko/distribution/Distribution.java_sec2
> [2]
> https://dxr.mozilla.org/mozilla-central/rev/
> 95ffbc4ff63584631c408e8d9912961fcf68bb09/mobile/android/base/java/org/
> mozilla/gecko/mozglue/NativeZip.java#73-85

I figured out libraries under /APK!/assets/ABI are szip'ed here [1] by [2] before packaging into APK, and they're not zip'ed again when packaging actually happens. 
So it's not able to unzip them by methods provided now in NativeZip.java or others.

IIUC, in nsGeckoUtils.cpp, it seems to be able to provide a helper function to inflate the szip'ed stream to ByteBuffer by 
Opt 1.
1. Creating a "SeekableZStream" zstream.
2. |mmap| it into memory and obtain a "MappedPtr" pointer.
3. |Decompress| the zstream to the that pointer.

or 
Opt 2.
1. Creating a MappableSeekableZStream.
2. Providing a new method which returns ByteBuffer to decompress zStream to |mozilla::UniquePtr<_MappableBuffer> buffer| by calling |zStream.Decompress|.

or
Opt 3.
Compressiong the libclearkey.so by [3] (not szip), so that it would be easier to uncompress it by existing util-function.

Hi glandium, I think Opt 2 is better here. But maybe there's an Opt 4 or more ...
it will be appreciated if you would give me some advices, thanks.


*NOTE* 
lib{mozglue,plugin-container-pie,plugin-container}.so are compressed by mozpack.mozjar.Deflater [3].

[1] https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/action/package_fennec_apk.py#66-79
[2] https://dxr.mozilla.org/mozilla-central/source/mozglue/linker/szip.cpp
[3] https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozpack/copier.py#524-529

Comment 29

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Kilik Kuo [:kikuo] from comment #28)
> (In reply to Kilik Kuo [:kikuo] from comment #27)
> > (In reply to Chris Pearce (:cpearce) from comment #25)
> > > Can you do one of the following:
> > > 
> > > 1. unzip gmp-clearkey to somewhere in the profile dir in when starting up
> > > GMPProvider if it's not yet been unzipped on startup.
> > > 2. unzip gmp-clearkey to somewhere in the profile dir when ClearKey is used
> > > for the first time. Possibly as part of resolving the
> > > MediaKeySystemAccess.createMediaKeys() promise.
> > > 
> > > Then you could use the existing code as-is, right? Wouldn't that be simpler?
> > > 
> > > I'd prefer option 2.
> > > 
> > > I'd prefer to avoid having a special path for gmp-clearkey on Android.
> > 
> > Considering to decompress the zipped stream, maybe I should use java class
> > "ZipInputStream" as the container of "FileInputStream". (not familiar with
> > Java:()
> > Also, I'll give this a try [2].
> > 
> > [1]
> > https://bugzilla.mozilla.org/attachment.cgi?id=8767487&action=diff#a/mobile/
> > android/base/java/org/mozilla/gecko/distribution/Distribution.java_sec2
> > [2]
> > https://dxr.mozilla.org/mozilla-central/rev/
> > 95ffbc4ff63584631c408e8d9912961fcf68bb09/mobile/android/base/java/org/
> > mozilla/gecko/mozglue/NativeZip.java#73-85
> 
> I figured out libraries under /APK!/assets/ABI are szip'ed here [1] by [2]
> before packaging into APK, and they're not zip'ed again when packaging
> actually happens. 
> So it's not able to unzip them by methods provided now in NativeZip.java or
> others.
> 
> IIUC, in nsGeckoUtils.cpp, it seems to be able to provide a helper function
> to inflate the szip'ed stream to ByteBuffer by 
> Opt 1.
> 1. Creating a "SeekableZStream" zstream.
> 2. |mmap| it into memory and obtain a "MappedPtr" pointer.
> 3. |Decompress| the zstream to the that pointer.
> 
> or 
> Opt 2.
> 1. Creating a MappableSeekableZStream.
> 2. Providing a new method which returns ByteBuffer to decompress zStream to
> |mozilla::UniquePtr<_MappableBuffer> buffer| by calling |zStream.Decompress|.
> 
> or
> Opt 3.
> Compressiong the libclearkey.so by [3] (not szip), so that it would be
> easier to uncompress it by existing util-function.
> 

Opt 4. 
It is much easier to obtain an unszip file by providing a helper function in nsGeckoUtls.cpp

pseudo code
...
void extract_szip_lib_to_target_place(const char *srcPath, const char* targetPath)
{
putenv("MOZ_LINKER_EXTRACT=1");

ElfLoader::GetMappableFromPath(srcPath);
MOVE_CACHED_LIB_TO_TARGET_DIR(targetPath);
DELETE_CACHED_LIB();

putenv("MOZ_LINKER_EXTRACT=0");
}
...

I think this is the most easiest/safest way to extract the szip'ed lib out of APK!.
But the question is ... I'm not sure if there's any consideration regarding the env variable "MOZ_LINKER_EXTRACT".   Any story ?

Comment 30

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Extract clearkey plugin related fliles when needed.

Hello Mike & Chris, 

This patch includes the following.

a) Helper function in Mappable.{h,cpp} to inflate szip'ed stream to a destination file.
b) In BrowserApp, preparing task and posting it to background thread, starting the extraction from BrowserApp.java => GeckoJarReader.java => NatvieZip.java => nsGeckoUtils.cpp => Mappable.cpp.
c) Setup the notification/observation mechanism between GMPProvider.jsm & BrowserApp.java

I think this should be the suitable way to enable gmp-clearkey on Fennec, please ignore previous hacky patches : )
It would be appreciated if you have feedback/advices for me.
Thanks.

Comment 31

[Chris Pearce [:cpearce (Not reading bugmail)]]

Can you use nsIZipReader to read the JAR/ZIP file? GMPInstallManager uses it:
https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/GMPInstallManager.jsm#389

So can you do the extraction in JavaScript in GMPProvider.jsm?

Comment 32

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Chris Pearce (:cpearce) from comment #31)
> Can you use nsIZipReader to read the JAR/ZIP file? GMPInstallManager uses it:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/
> GMPInstallManager.jsm#389
> 
> So can you do the extraction in JavaScript in GMPProvider.jsm?

Sadly, *.so under /APK!/assets/ are szip'ed by another independent c++ program [1], and they are just packaged into APK with zlib compression method "STORED" via Jarrer [2], which means no further compression is performed.

The JAR/ZIP extraction procedure in GMPInstallManager, which is performed here [3], is based on zlib  with the compression method "DEFLATED".

So we cannot extract the szip'ed libraries by zlib-based JARReader.
One possible solution is to exceptionally handle libclearkey.so during packaging, i.e either (a) not placing it under /APK!/assets/ (b) check explicitly for clearkey here [4], so that libclearkey.so can be compressed by zlib rather than szip.  
Although I'd prefer providing a general extraction method for szip'ed library.

Any advices, Mike.

[1] https://dxr.mozilla.org/mozilla-central/source/mozglue/linker/szip.cpp
[2] https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozpack/copier.py#474
[3] https://dxr.mozilla.org/mozilla-central/source/modules/libjar/nsZipArchive.cpp#1210-1255
[4] https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/action/package_fennec_apk.py#66-79

Comment 33

[Mike Hommey [:glandium]]

Considering bug 1291424, let's pause here.

Comment 34

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Mike Hommey [:glandium] from comment #33)
> Considering bug 1291424, let's pause here.

Thanks Mike, it seems we don't have a clear schedule for bug 1291424, hmm..

Hey Chris, since you've mentioned to leverage the nsZipReader in GMPInstallManager.jsm, another idea came up to me. Is it possible to treat clearkey as a downloadable plugin, like gmp-openh264, so that Fennec could download it after start-up or at the time when needs it, instead of packing it into APK.  Is it doable ?

Comment 35

[Chris Pearce [:cpearce (Not reading bugmail)]]

We could host ClearKey GMP for Android on a Mozilla controlled server, and download it from there. However it would be a lot simpler (in terms of organizational overhead) to maintain ClearKey if the code for it was just in mozilla-central and part of the build.

Bug 1291424 was only filed today, so it's premature to reason about how long it's going to take. However an estimate would be nice... Mike?

Let's be clear what we're trying to achieve with ClearKey. The main benefit we get is not users using ClearKey, since we don't expect actual content to be decrypted with it.

The first benefit is us being able to test the plugin API that our actual DRM systems implement/use.

The second benefit is our EME JS APIs can be tested against other browser's implementations which also implement ClearKey.

Given that we'll be using MediaDRM (or what it MediaCodec? I can't remember...) to do Widevine on Android, using ClearKey as a GMP means we won't get the first benefit. So I'm wondering if the best solution here is to actually write an adapter that makes our ClearKey implementation work with the same interface that the Widevine CDM uses on Android. How much work is that? That would be blocked on a bunch of other work, so maybe this should be the end goal, rather than what we do first to get ClearKey working.

Yet another option would be to statically link the ClearKey code into libxul.so on Android, and then write a GMPAdapter which calls this code statically from inside the libxul.so that gets loaded in the child process. You'd need to add stuff to GMPParent to allow initializing the GMPParent without a path.

If Bug 1291424 is resolved in a few weeks, can you wait on that kilik? If not, we should clean up whichever option of those you've explored so far that has required the least invasive changes.

Comment 36

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Chris Pearce (:cpearce) from comment #35)
> We could host ClearKey GMP for Android on a Mozilla controlled server, and
> download it from there. However it would be a lot simpler (in terms of
> organizational overhead) to maintain ClearKey if the code for it was just in
> mozilla-central and part of the build.

I think so. 

> 
> Bug 1291424 was only filed today, so it's premature to reason about how long
> it's going to take. However an estimate would be nice... Mike?
> 
> Let's be clear what we're trying to achieve with ClearKey. The main benefit
> we get is not users using ClearKey, since we don't expect actual content to
> be decrypted with it.
> 
> The first benefit is us being able to test the plugin API that our actual
> DRM systems implement/use.
> 
> The second benefit is our EME JS APIs can be tested against other browser's
> implementations which also implement ClearKey.
> 
> Given that we'll be using MediaDRM (or what it MediaCodec? I can't
> remember...) to do Widevine on Android, using ClearKey as a GMP means we
> won't get the first benefit. So I'm wondering if the best solution here is
> to actually write an adapter that makes our ClearKey implementation work
> with the same interface that the Widevine CDM uses on Android. How much work
> is that? That would be blocked on a bunch of other work, so maybe this
> should be the end goal, rather than what we do first to get ClearKey working.
> 

On Android, MediaDRM API(Java API) is used for applications to obtain a BpInterface(Binder proxy) in order to achieve IPC with the MeidaPlayerService, which contains a BnInterface(Binder native).
Proprietary DRM libraries, which are required to subclass IDrm/ICrypto interfaces [1], should be located in /system/vendor/lib/mediadrm/, so that MediaPlayerService (service in another process rather than app process) can find supported plugin inside that folder.  Even we implement an adaptor finally (like James & I did on B2G), AFAIK, applications are not able to put libraries into that *vendor* folder.
So this might not work.

[1] http://androidxref.com/5.1.1_r6/xref/frameworks/av/include/media/IDrm.h

> Yet another option would be to statically link the ClearKey code into
> libxul.so on Android, and then write a GMPAdapter which calls this code
> statically from inside the libxul.so that gets loaded in the child process.
> You'd need to add stuff to GMPParent to allow initializing the GMPParent
> without a path.
> 

That's a way that James & I discussed before, but this makes clearkey become an exception in the framework.   First it breaks my understanding of the guide of GMP Plugin (there should be SOME.so & SOME.info in plugin folder). Second, we make fennec-clearkey into a 3rd (not like original GMP, not like desktop-widevine) model.  Third, if we did this on fennec, then wouldn't it be better to sync clearkey's behaviour on desktop too ?   IIUC, that would make code more complicated, and I'd prefer not to.

> If Bug 1291424 is resolved in a few weeks, can you wait on that kilik? If

Sure, I can wait, this issue is not that urgent, and for me, there's little benefit.
So I also want to find a way that would leverage current framework with the least invasive changes.
If the on-demand (szip) decompression is deprecated, or it is allowed to compress libraries by "zlib" during packaging, the changes should be much less : )

> not, we should clean up whichever option of those you've explored so far
> that has required the least invasive changes.

BTW, thanks for the explanation, Chris. It's been a detour for me exploring these solutions, but it's also exciting to get to know better.  I'll talk a breath here.

Comment 37

[Mike Hommey [:glandium]]

(In reply to Kilik Kuo [:kikuo] from comment #34)
> (In reply to Mike Hommey [:glandium] from comment #33)
> > Considering bug 1291424, let's pause here.
> 
> Thanks Mike, it seems we don't have a clear schedule for bug 1291424, hmm..

I'm expecting bug 1294731 will land next week. Bug 1291424 should follow soon after.

Comment 38

[James Cheng[:JamesCheng]]

I will continue to work on this bug.

Comment 39

[James Cheng[:JamesCheng]]

Attachment: Bug 1267141 - Part1 - Pack clearkey info and lib into APK asset folder.

Review commit: https://reviewboard.mozilla.org/r/105042/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105042/

Comment 40

[James Cheng[:JamesCheng]]

Attachment: Bug 1267141 - Part2 - Make extractGeckoLibsNative be a generic function not only for extracting libxul.

Review commit: https://reviewboard.mozilla.org/r/105044/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105044/

Comment 41

[James Cheng[:JamesCheng]]

Currently, EME API for widevine DRM is enabled on Nightly but clearkey did not.

We would like to enable the clearkey CDM and enable the related test case to ensure the Web API works.

Comment 42

[Mike Hommey [:glandium]]

Comment on attachment 8827325 [details]
Bug 1267141 - Part1 - Pack clearkey info and lib into APK asset folder.

https://reviewboard.mozilla.org/r/105042/#review106136

Comment 43

[Mike Hommey [:glandium]]

Comment on attachment 8827326 [details]
Bug 1267141 - Part2 - Make extractGeckoLibsNative be a generic function not only for extracting libxul.

https://reviewboard.mozilla.org/r/105044/#review106140

I'm not the right reviewer for the java/jsm parts. But I also don't know who would be a good reviewer that.

::: mozglue/android/APKOpen.cpp:354
(Diff revision 1)
>  
>    jenv->ReleaseStringUTFChars(jApkName, apkName);
>  }
>  
>  extern "C" APKOPEN_EXPORT void MOZ_JNICALL
> +Java_org_mozilla_gecko_mozglue_GeckoLoader_extractClearkeyLibNative(

It seems to me this function should be merged with extractGeckoLibsNative, to form a single, generic, function to extract libraries.

Comment 44

[James Cheng[:JamesCheng]]

Attachment: Bug 1267141 - Part3 - Extracting clearkey library and info files into data cache folder.

Review commit: https://reviewboard.mozilla.org/r/105346/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105346/

Comment 45

[James Cheng[:JamesCheng]]

Comment on attachment 8827325 [details]
Bug 1267141 - Part1 - Pack clearkey info and lib into APK asset folder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105042/diff/1-2/

Comment 46

[James Cheng[:JamesCheng]]

Comment on attachment 8827326 [details]
Bug 1267141 - Part2 - Make extractGeckoLibsNative be a generic function not only for extracting libxul.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105044/diff/1-2/

Comment 47

[James Cheng[:JamesCheng]]

Hi glandium,
As your comment said, I create part2 to make it generic.

And for jsm part, I would like Chris' help to do the review.

Thank you!

Comment 48

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8827731 [details]
Bug 1267141 - Part3 - Extracting clearkey library and info files into data cache folder.

https://reviewboard.mozilla.org/r/105346/#review106462

The approach here looks fine to me, but I'm not the right person to review changes to these files.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:65
(Diff revision 1)
>  import org.mozilla.gecko.icons.Icons;
>  import org.mozilla.gecko.javaaddons.JavaAddonManager;
>  import org.mozilla.gecko.media.VideoPlayer;
>  import org.mozilla.gecko.menu.GeckoMenu;
>  import org.mozilla.gecko.menu.GeckoMenuItem;
> +import org.mozilla.gecko.mozglue.GeckoLoader;

I'm not the right person to review changes to BrowserApp.java.

`hg log mobile/android/base/java/org/mozilla/gecko/BrowserApp.java` will probably give you a good idea of who normally reviews code in this file.
  `

::: toolkit/mozapps/extensions/internal/GMPProvider.jsm:23
(Diff revision 1)
>  Cu.import("resource://gre/modules/osfile.jsm");
>  /* globals OS*/
>  Cu.import("resource://gre/modules/Log.jsm");
>  Cu.import("resource://gre/modules/Task.jsm");
>  Cu.import("resource://gre/modules/GMPUtils.jsm");
>  /* globals EME_ADOBE_ID, GMP_PLUGIN_IDS, GMPPrefs, GMPUtils, OPEN_H264_ID, WIDEVINE_ID */

Changes to GMPProvider.jsm should be reviewed by spohl.

Comment 49

[Mike Hommey [:glandium]]

Comment on attachment 8827326 [details]
Bug 1267141 - Part2 - Make extractGeckoLibsNative be a generic function not only for extracting libxul.

https://reviewboard.mozilla.org/r/105044/#review106508

Comment 50

[Mike Hommey [:glandium]]

I'm still not the right person to review the java and jsm code.

Comment 51

[James Cheng[:JamesCheng]]

Thanks Chris' and Mike's review and feedback, I will try to find the right reviewer.

Hi spohl, please help to review the modification of GMPProvider.jsm, thanks.

Hi Sebastian, could you please review the BrowserApp.java part? Thank you.

Comment 52

[Stephen A Pohl [:spohl]]

Comment on attachment 8827731 [details]
Bug 1267141 - Part3 - Extracting clearkey library and info files into data cache folder.

https://reviewboard.mozilla.org/r/105346/#review106706

r+ on GMPProvider.jsm with comments addressed.

::: toolkit/mozapps/extensions/internal/GMPProvider.jsm:551
(Diff revision 1)
> +        this._log.info("Receiving topic - 'EME:ExtractClearkeyDone'");
> +        if (aData) {
> +          gmpService.addPluginDirectory(aData);
> +        } else {
> +          this._log.info("EME:ExtractClearkeyDone - adding gmp directory failed ");
> +        }

add |break;| at end of this case

::: toolkit/mozapps/extensions/internal/GMPProvider.jsm:622
(Diff revision 1)
> +      } else {
> +        clearkeyPath = OS.Path.join(greDir.path,
> -                                      CLEARKEY_PLUGIN_ID,
> +                                    CLEARKEY_PLUGIN_ID,
> -                                      CLEARKEY_VERSION);
> +                                    CLEARKEY_VERSION);
> -      this._log.info("startup - adding clearkey CDM directory " +
> +        this._log.info("startup - adding clearkey CDM directory " +
> -                     clearkeyPath);
> +               clearkeyPath);

nit: fix indent of this line to match new indent.

Comment 53

[James Cheng[:JamesCheng]]

Comment on attachment 8827325 [details]
Bug 1267141 - Part1 - Pack clearkey info and lib into APK asset folder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105042/diff/2-3/

Comment 54

[James Cheng[:JamesCheng]]

Comment on attachment 8827326 [details]
Bug 1267141 - Part2 - Make extractGeckoLibsNative be a generic function not only for extracting libxul.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105044/diff/2-3/

Comment 55

[James Cheng[:JamesCheng]]

Comment on attachment 8827731 [details]
Bug 1267141 - Part3 - Extracting clearkey library and info files into data cache folder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105346/diff/1-2/

Comment 56

[James Cheng[:JamesCheng]]

Fix the review feedback and update the patch for the change of Bug 1318965.

Comment 57

[James Cheng[:JamesCheng]]

Attach the treeherder result. 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d91c5983cf63882e690a28a9111c91c9aca4903d

Comment 58

[James Cheng[:JamesCheng]]

Comment on attachment 8827325 [details]
Bug 1267141 - Part1 - Pack clearkey info and lib into APK asset folder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105042/diff/3-4/

Comment 59

[James Cheng[:JamesCheng]]

Comment on attachment 8827326 [details]
Bug 1267141 - Part2 - Make extractGeckoLibsNative be a generic function not only for extracting libxul.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105044/diff/3-4/

Comment 60

[James Cheng[:JamesCheng]]

Comment on attachment 8827731 [details]
Bug 1267141 - Part3 - Extracting clearkey library and info files into data cache folder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105346/diff/2-3/

Comment 61

[Sebastian Kaspari (:sebastian; :pocmo)]

@esawin: You recently worked on extracting libxul - so maybe you have an opinion here too?

Comment 62

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8827731 [details]
Bug 1267141 - Part3 - Extracting clearkey library and info files into data cache folder.

https://reviewboard.mozilla.org/r/105346/#review107786

From looking at the code I have some questions.

* How do we handle updated versions of the library? Do we extract them if we already have an older version?
* Do we always ship this library or is this behind a build flag -> should this code be behind a build flag too?
* Is the cache directory the best place for this? Android may decide to delete the files to recover space. But I guess we'd just re-extract the next time we need it?
* Some code cleanup would be good: Use 'final' where applicable, catch specific exceptions to avoid hiding valid errors, move generic IO code to FileUtils.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:601
(Diff revision 3)
>              return true;
>          }
>          return super.onKeyUp(keyCode, event);
>      }
>  
> +    private void ensureParentDirExist(final File file) throws Exception {

This method could be moved to FileUtils. BrowserApp is already a monster and it would be nice to not make it much bigger. :)

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:602
(Diff revision 3)
>          }
>          return super.onKeyUp(keyCode, event);
>      }
>  
> +    private void ensureParentDirExist(final File file) throws Exception {
> +        File oParentDir = file.getParentFile();

nit: final

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:604
(Diff revision 3)
>      }
>  
> +    private void ensureParentDirExist(final File file) throws Exception {
> +        File oParentDir = file.getParentFile();
> +        if (!oParentDir.exists()) {
> +            Log.i(LOGTAG, "Creating " + oParentDir.getAbsolutePath());

There are a bunch log statements in the code. They probably have been helpful while writing and testing the code, but do they need to be logged on every device going forward?

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:606
(Diff revision 3)
> +    private void ensureParentDirExist(final File file) throws Exception {
> +        File oParentDir = file.getParentFile();
> +        if (!oParentDir.exists()) {
> +            Log.i(LOGTAG, "Creating " + oParentDir.getAbsolutePath());
> +            if (!oParentDir.mkdirs()) {
> +                String description = "Unable to create directories: " + oParentDir.getAbsolutePath();

nit: final

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:607
(Diff revision 3)
> +        File oParentDir = file.getParentFile();
> +        if (!oParentDir.exists()) {
> +            Log.i(LOGTAG, "Creating " + oParentDir.getAbsolutePath());
> +            if (!oParentDir.mkdirs()) {
> +                String description = "Unable to create directories: " + oParentDir.getAbsolutePath();
> +                throw new Exception(description);

Do we really want to throw and crash the app in this situation?

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:616
(Diff revision 3)
> +
> +    /**
> +    * Copies files under /assets/@ANDROID_CPU_ARCH@/PLUGIN_NAME/PLUGIN_VER out
> +    * of the APK and into the app's cache directory.
> +    */
> +    private void extractClearKeyFiles(final String dstClearkeyPath) {

nit: Some 'final's here and there

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:647
(Diff revision 3)
> +        String dstLibPath = dstClearkeyPath + File.separator + libName;
> +        String dstInfoPath = dstClearkeyPath + File.separator + infoName;
> +
> +        try {
> +            final File oDstLibFile = new File(dstLibPath);
> +            if (!oDstLibFile.exists()) {

What if we ship a new version of the lib. Doesn't this prevent us from ever extracting the new version?

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:657
(Diff revision 3)
> +                                                 pkgPath,
> +                                                 libPath);
> +                    // Move extracted lib to the gmp-clearkey folder.
> +                    File from = new File(getContext().getCacheDir(), libName);
> +                    from.renameTo(oDstLibFile);
> +                } catch (Throwable e) {

Cathing Throwable here and later seems to be a bit excessive. Can we be more specific and catch the actual exception that we think happen here?

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:665
(Diff revision 3)
> +            }
> +
> +            final File oInfoFile = new File(dstInfoPath);
> +            if (!oInfoFile.exists()) {
> +                // Extract compressed .info file to destination path.
> +                java.io.InputStream inStream = GeckoJarReader.getStream(getContext(),

nit: No need for the java.io.* prefix - here and two lines down.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:668
(Diff revision 3)
> +                try {
> +                    IOUtils.copy(inStream, outStream);
> +                } catch (Throwable e) {

You only need to catch IOException here.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:673
(Diff revision 3)
> +                    inStream.close();
> +                    outStream.close();

Consider using IOUtils.safeStreamClose() here. close() can throw an exception and then you wouldn't close the second stream.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:677
(Diff revision 3)
> +                } finally {
> +                    inStream.close();
> +                    outStream.close();
> +                }
> +            }
> +            GeckoAppShell.notifyObservers("EME:ExtractClearkeyDone", dstClearkeyPath);

Following the patch above it could happen that we didn't extract the library (e.g. copy fails) and yet we notify a success here?

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1869
(Diff revision 3)
>                  break;
>  
> +            case "EME:ExtractClearkey":
> +                Log.i(LOGTAG, "Event=>'EME:ExtractClearkey' ... posting task");
> +                final String dstClearkeyDir = message.getString("targetpath");
> +                ThreadUtils.postToBackgroundThread(new Runnable() {

If this should happen on a background thread then register this event on a background thread (registerBackgroundThreadListener instead of registerGeckoThreadListener).

Comment 63

[James Cheng[:JamesCheng]]

(In reply to Sebastian Kaspari (:sebastian) from comment #62)
> Comment on attachment 8827731 [details]
> Bug 1267141 - Part3 - Extracting clearkey library and info files into data
> cache folder.
> 
> https://reviewboard.mozilla.org/r/105346/#review107786
> 
> From looking at the code I have some questions.
> 
> * How do we handle updated versions of the library? Do we extract them if we
> already have an older version?
> * Do we always ship this library or is this behind a build flag -> should
> this code be behind a build flag too?
I need to confirm with cpearce for above two questions since desktop Firefox is shipped this cdm in production build.

Hi Chris, 
Two questions needed to confirm with you.
1. Will clearkey version be updated? Or it is updated rarley(may be never). If so, I prefer to do it in a follow up bug.
2. Is it worth shipping this clearkey.so in release build for fennec?
If not, I think I should stop working on this bug and focus on higher priority task.
> * Is the cache directory the best place for this? Android may decide to
> delete the files to recover space. But I guess we'd just re-extract the next
> time we need it?
I am not sure, I just saw the other .so libraries are extracted there and I just obey the rules. 
> * Some code cleanup would be good:
I will address those feedback, thank you very much.

Comment 64

[Eugen Sawin [:esawin]]

Comment on attachment 8827731 [details]
Bug 1267141 - Part3 - Extracting clearkey library and info files into data cache folder.

https://reviewboard.mozilla.org/r/105346/#review107844

(In reply to Sebastian Kaspari (:sebastian) from comment #62)
> * How do we handle updated versions of the library? Do we extract them if we
> already have an older version?

The linker validates cached extracted libraries based on their checksums as provided by the APK.
The check on the Java side would indeed prevent potential upgrades of the lib, so it should be removed.

> * Is the cache directory the best place for this? Android may decide to
> delete the files to recover space. But I guess we'd just re-extract the next
> time we need it?

That's the status quo with libxul, so it should be OK for this case, too.

Comment 65

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to James Cheng[:JamesCheng] OoO from 1/27 - 2/1 from comment #63)
> 1. Will clearkey version be updated? Or it is updated rarley(may be never).
> If so, I prefer to do it in a follow up bug.

I'm not sure what you're asking. We don't strongly version gmp-clearkey; the version number in manifest.json is pretty much just a placeholder. Given that we build clearkey at the same time as Firefox from mozilla-central, it doesn't make sense to version Firefox and clearkey separately.

We need to maintain the ability to patch our own code. So we may need to change the lib extracted here after a Firefox update.

> 2. Is it worth shipping this clearkey.so in release build for fennec?
> If not, I think I should stop working on this bug and focus on higher
> priority task.

It's worth having clearkey working so that we can demonstrate interoperability. But EME on Fennec isn't super-high priority. So if you've got something more important to work on, work on that.

Comment 66

[James Cheng[:JamesCheng]]

(In reply to Eugen Sawin [:esawin] from comment #64)
> Comment on attachment 8827731 [details]
> Bug 1267141 - Part3 - Extracting clearkey library and info files into data
> cache folder.
> 
> https://reviewboard.mozilla.org/r/105346/#review107844
> 
> (In reply to Sebastian Kaspari (:sebastian) from comment #62)
> > * How do we handle updated versions of the library? Do we extract them if we
> > already have an older version?
> 
> The linker validates cached extracted libraries based on their checksums as
> provided by the APK.
> The check on the Java side would indeed prevent potential upgrades of the
> lib, so it should be removed.
> 
Do you mean that extractGeckoLibs will do the checksum validation inside so that I should always call this function to ensure the lib is updated? 

Another question is that is there any preference or ways that i can use to check there is a app update from marketplace? 

Thank you very much. 
> > * Is the cache directory the best place for this? Android may decide to
> > delete the files to recover space. But I guess we'd just re-extract the next
> > time we need it?
> 
> That's the status quo with libxul, so it should be OK for this case, too.

Comment 67

[Eugen Sawin [:esawin]]

(In reply to James Cheng[:JamesCheng] OoO from 1/27 - 2/1 from comment #66)
> Do you mean that extractGeckoLibs will do the checksum validation inside so
> that I should always call this function to ensure the lib is updated?

Fennec calls extractGeckoLibs on every startup and the linker takes care of extraction if required.
However, since you move the file out of the cache directory into the clearkey directory, the linker would have to extract it on every startup.

If possible, you should use the library directly out of the cache folder instead.
Otherwise, you would have to re-implement some checksum verification analogue to what the linker does with the CacheValidator [1].

> Another question is that is there any preference or ways that i can use to
> check there is a app update from marketplace? 

I don't think that's something we should or have to do manually, Sebastian?

[1] https://searchfox.org/mozilla-central/source/mozglue/linker/Mappable.cpp#31

Comment 68

[James Cheng[:JamesCheng]]

(In reply to Eugen Sawin [:esawin] from comment #67)
> (In reply to James Cheng[:JamesCheng] OoO from 1/27 - 2/1 from comment #66)
> > Do you mean that extractGeckoLibs will do the checksum validation inside so
> > that I should always call this function to ensure the lib is updated?
> 
> Fennec calls extractGeckoLibs on every startup and the linker takes care of
> extraction if required.
> However, since you move the file out of the cache directory into the
> clearkey directory, the linker would have to extract it on every startup.
> 
> If possible, you should use the library directly out of the cache folder
> instead.
I see.

Currently I extract lib and manifest into ```cache/gmp-clearkey/0.1``` because of the GMP implementation here [1] is to read the folder name.

I don't want to write more specific code to handle the checksum validation only for this case and that seems a big effort.

Another alternative I think of is to extract the lib into ```cache\``` and make a symbolic link into 
```cache/gmp-clearkey/0.1```

But I cannot find java API that I can make a symbolic link, do you have any idea of this?

Thank you for your advice and feedback!

[1]
http://searchfox.org/mozilla-central/rev/d20e4431d0cf40311afa797868bc5c58c54790a2/dom/media/gmp/GMPParent.cpp#117-132

> Otherwise, you would have to re-implement some checksum verification
> analogue to what the linker does with the CacheValidator [1].
> 
> > Another question is that is there any preference or ways that i can use to
> > check there is a app update from marketplace? 
> 
> I don't think that's something we should or have to do manually, Sebastian?
> 
> [1]
> https://searchfox.org/mozilla-central/source/mozglue/linker/Mappable.cpp#31

Comment 69

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Eugen Sawin [:esawin] from comment #67)
> > Another question is that is there any preference or ways that i can use to
> > check there is a app update from marketplace? 
> 
> I don't think that's something we should or have to do manually, Sebastian?

Right. There are multiple sources for updates (Sideload, Google Play, Our own updater, other markets, ..) so it's impossible to know if there's an update waiting or incoming before actually installing it.

Comment 70

[Eugen Sawin [:esawin]]

(In reply to James Cheng[:JamesCheng] from comment #68)
> Currently I extract lib and manifest into ```cache/gmp-clearkey/0.1```
> because of the GMP implementation here [1] is to read the folder name.

Would it be possible to make that configurable?

> Another alternative I think of is to extract the lib into ```cache\``` and
> make a symbolic link into 
> ```cache/gmp-clearkey/0.1```
> 
> But I cannot find java API that I can make a symbolic link, do you have any
> idea of this?

The required function [1] is only available on API 21+. I assume you could achieve it using reflection or native code, although I'm not sure what our policy is on that, you would have to ask Sebastian.

[1] https://developer.android.com/reference/android/system/Os.html#symlink(java.lang.String,%20java.lang.String)

Comment 71

[Nick Alexander :nalexander [he/him]]

(In reply to Eugen Sawin [:esawin] from comment #70)
> (In reply to James Cheng[:JamesCheng] from comment #68)
> > Currently I extract lib and manifest into ```cache/gmp-clearkey/0.1```
> > because of the GMP implementation here [1] is to read the folder name.
> 
> Would it be possible to make that configurable?
> 
> > Another alternative I think of is to extract the lib into ```cache\``` and
> > make a symbolic link into 
> > ```cache/gmp-clearkey/0.1```
> > 
> > But I cannot find java API that I can make a symbolic link, do you have any
> > idea of this?
> 
> The required function [1] is only available on API 21+. I assume you could
> achieve it using reflection or native code, although I'm not sure what our
> policy is on that, you would have to ask Sebastian.

Just FYI, you can probably use this on all APIs from native code quite easily.  However, I believe that not all Android file systems actually support symlinks, although I don't have a source to hand.  Just remember that there are exotic devices with "exciting" configurations...

Comment 72

[James Cheng[:JamesCheng]]

Comment on attachment 8827325 [details]
Bug 1267141 - Part1 - Pack clearkey info and lib into APK asset folder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105042/diff/4-5/

Comment 73

[James Cheng[:JamesCheng]]

Comment on attachment 8827326 [details]
Bug 1267141 - Part2 - Make extractGeckoLibsNative be a generic function not only for extracting libxul.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105044/diff/4-5/

Comment 74

[James Cheng[:JamesCheng]]

Comment on attachment 8827731 [details]
Bug 1267141 - Part3 - Extracting clearkey library and info files into data cache folder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105346/diff/3-4/

Comment 75

[James Cheng[:JamesCheng]]

Comment on attachment 8827731 [details]
Bug 1267141 - Part3 - Extracting clearkey library and info files into data cache folder.

Just cancel the review.

Thanks for all the feedback and I decide to postpone investigating on this bug.

Update the patch including

1. Addressed the nit.
2. Always allow extracgeckolib to check the lib version.
3. Make a symbolic link into gmp-clearkey/0.1/ for GMP architecture.(As comment 71 said, some legacy or exotic devices did not support symlink on its file system)

The patch works well on my local device.
It is nice to have someone make the bug landable.

Comment 77

[u645759]

If this the bug tracking the implementation of Widevine for Fennec? It hasn't been updated in 2 years. Is this really the situation with Widewine for Firefox Android? Still not supported?

Comment 78

[Sylvestre Ledru [:Sylvestre]]

As we are now focusing on Fenix (firefox preview), I don't think this will happen for Fennec.

Comment 79

[u645759]

@sylvestre: Thanks! And the bug tracking the Widevine implementation in Fenix is here