Closed Bug 1341990 Opened 7 years ago Closed 7 years ago

Introduce Exoplayer into build system

Categories

(Firefox Build System :: Android Studio and Gradle Integration, defect)

Product:
Firefox Build System
Component:
Android Studio and Gradle Integration
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: JamesCheng, Assigned: kikuo)

References

(Depends on 1 open bug)

Details

Attachments

(7 files, 7 obsolete files)

Bug 1341990 - Part 0: Add a feature flag to control source code build time coverage for ExoPlayer.
59 bytes, text/x-review-board-request
nalexander
: review+
Details
0001-Respect-the-ProGuard-rules-for-ExoPlayer.patch
1.61 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
Bug 1341990 - Part 1: Import ExoPlayer sources (r2.4.0) without {ui/,dash/,smoothstreaming/,all/} subdirectories.
59 bytes, text/x-review-board-request
nalexander
: review+
Details
Bug 1341990 - Part 2: Make ExoPlayer (r2.4.0) build against Android SDK 23.
59 bytes, text/x-review-board-request
nalexander
: review+
Details
Bug 1341990 - Part 3: Include ExoPlayer in Firefox for Android builds.
59 bytes, text/x-review-board-request
nalexander
: review+
Details
Bug 1341990 - Post: Move ExoPlayer to GeckoView thirdparty source directory.
59 bytes, text/x-review-board-request
maliu
: review+
Details
Bug 1341990 - Post: Update build system to reference ExoPlayer in GeckoView thirdparty source directory.
59 bytes, text/x-review-board-request
maliu
: review+
Details 
We want to leverage the functionality provided by Google Exoplayer[1] to implement HLS  on Fennec.

The library is hosted on jcenter.

So we want to find a proper way to import the library into our gecko build system.

[1] Exoplayer
https://github.com/google/ExoPlayer
Currently, we can use this patch to import Exoplayer library and use it on Gecko.

We only can use one of the below commands to build the APK locally(by using gradle build),
(1) ./mach configure --with-gradle && ./mach build && ./mach package && ./mach install
(2) ./mach build binaries (build only C++) && ./gradlew build (build java part) && deploy the apk by adb install

We would like to know how to use this kind of library without gradle build in gecko.

Our final goal is to achieve
(1) only using ```./mach build ``` and everything can be done.
(2) to make build server build successfully even without gradle build.
Hi Jim,

According to the comment 1,

Do you know who is the export in this area?

Is there any similar library in gecko that we can take reference?

We would like his help to make this bug move on since we're not familiar with the build system and the library usage on Fennec.

Thank you.
Flags: needinfo?(nchen)
Hi Jim,

According to the comment 1,

Do you know who is the expert in this area?

Is there any similar library in gecko that we can take reference?

We would like his help to make this bug move on since we're not familiar with the build system and the library usage on Fennec.

Thank you.
Flags: needinfo?(nchen)
sorry for the typo, please see comment 3.
thanks.
Flags: needinfo?(nchen)
Typically it's :nalexander. However, I see the latest ExoPlayer AAR is about 1MB in size. That's most likely too large for us to ship with Fennec.
Flags: needinfo?(nchen) → needinfo?(nalexander)
(In reply to Jim Chen [:jchen] [:darchons] from comment #5)
> Typically it's :nalexander. However, I see the latest ExoPlayer AAR is about
> 1MB in size. That's most likely too large for us to ship with Fennec.

I think there's a balance between size and value (as always).  It's worth pushing a try build to figure out what really happens to our APK size, since the resource cost and the code cost (after Proguard) can be different.

Technically it appears that this library would be possible to integrate into Fennec without significant build work (which your patch may have already done, and I can help get across the line).
Flags: needinfo?(nalexander)
Hi Jim and nalexander, 
thanks for your help.

We are currently in a initial stage and still under feasibility study for HLS to support Fennec.

I just want to know if I want to put the library into our gecko build system (both local and server build),

how many things I need to take care of?

Is there any library that is similar to our case? I could try to take it as reference.

IIUC, there is only one gradle build(Android API15+ Gradle opt) on treeherder [1].

Is it possible to make it build success on the rest of build?

I'm not sure what's the configuration difference between Android API15+ Gradle opt and the others.

We would like your valuable guidance and indication.

Thank you very much.

[1] Server build
Android 4.0 API15+ opt
Android 4.0 API15+ debug 
Android 4.2 x86 opt
Android 4.2 API15+ opt   
Android 4.3 API15+ opt   
Android 4.3 API15+ debug
Android 4.4 API15+ opt   
Android 4.4 API15+ debug 
Android 5.1 API15+ opt   
Android 5.1 API15+ debug 
Android 6.0 API15+ opt   
Android 6.0 API15+ debug 
Android API15+ Gradle opt
Flags: needinfo?(nalexander)
sebastian: these aren't really ready for review, but I wanted you to be aware of them.  Feel free to make comments and r+ (for future application, maybe?) or r- as you see fit.

James: these should let you get moving with try builds for comparing sizes, etc.  Please read the commit comments to understand some of the things left to do, etc.
Flags: needinfo?(s.kaspari)
Flags: needinfo?(nalexander)
Flags: needinfo?(jacheng)
(In reply to Nick Alexander :nalexander from comment #11)
> sebastian: these aren't really ready for review, but I wanted you to be
> aware of them.  Feel free to make comments and r+ (for future application,
> maybe?) or r- as you see fit.
> 
> James: these should let you get moving with try builds for comparing sizes,
> etc.  Please read the commit comments to understand some of the things left
> to do, etc.

Is this the only way to get exoplayer2 included in Fennec ?
As we intend to link the library with a fixed version first, i.e. r2.1.1 to make it less risky for legal consideration, see Bug 1016449 Comment 75, Bug 1016449 Comment 76.

IIUC, I'm wondering if we could take the same way, like the way which Fennec links google-play-service  from pre-downloaded libraries in Android SDK [1] ?

[1] http://searchfox.org/mozilla-central/rev/90d1cbb4fd3dc249cdc11fe5c3e0394d22d9c680/mobile/android/app/build.gradle#209-222
(In reply to Kilik Kuo [:kikuo] from comment #12)
> (In reply to Nick Alexander :nalexander from comment #11)
> > sebastian: these aren't really ready for review, but I wanted you to be
> > aware of them.  Feel free to make comments and r+ (for future application,
> > maybe?) or r- as you see fit.
> > 
> > James: these should let you get moving with try builds for comparing sizes,
> > etc.  Please read the commit comments to understand some of the things left
> > to do, etc.
> 
> Is this the only way to get exoplayer2 included in Fennec ?
> As we intend to link the library with a fixed version first, i.e. r2.1.1 to
> make it less risky for legal consideration, see Bug 1016449 Comment 75, Bug
> 1016449 Comment 76.

I don't have access to either of those tickets.
 
> IIUC, I'm wondering if we could take the same way, like the way which Fennec
> links google-play-service  from pre-downloaded libraries in Android SDK [1] ?

This is equivalent, except here we have source code.  Source code that is licensed Apache -- how can this be legally questionable?

In any case, this seems _better_ than how Fennec integrates GPS.  We end up including a JAR, but for ExoPlayer we produce the JAR ourselves, since it's open source.  If you really want to, you can integrate a JAR that the ExoPlayer project produces, which is still licensed Apache.

I am very confused; perhaps the tickets you link to will help me understand what you're trying to achieve and what you're trying to avoid.
Hi Nick,

I've added you into Bug 1016449 cc list as Kilik mentioned in comment 12.

Thank you for providing this patch and we will continue to implement based on these patches.

However, if we can integrate the library like GPS did, it will be much simpler than add all the source code inside.
Flags: needinfo?(jacheng)
Flags: needinfo?(s.kaspari)
Comment on attachment 8841014 [details]
Bug 1341990 - Part 1: Import ExoPlayer sources, minus the ui/ directory.

https://reviewboard.mozilla.org/r/115368/#review117760

uh. that's a lot of code. r+ because it's just a code dump. but yeah, let's look at how this affects APK size.

just for understanding: what's the advantage of this exoplayer code over VideoView that we use today? will this allow us to get rid of the overlay approach and render hls videos "in content"? i'm just surprised that it's all java code.. :)
Attachment #8841014 - Flags: review?(s.kaspari) → review+
Comment on attachment 8841015 [details]
Bug 1341990 - Part 2: Make ExoPlayer build against Android SDK 23.

https://reviewboard.mozilla.org/r/115370/#review117764
Attachment #8841015 - Flags: review?(s.kaspari) → review+
Comment on attachment 8841016 [details]
Bug 1341990 - Part 3: Include ExoPlayer in Firefox for Android builds.

https://reviewboard.mozilla.org/r/115372/#review117766

Clearing the review here. The changeset is appearently okay - however I would like us to look at the size increment and the advantages we get from it before landing.
Attachment #8841016 - Flags: review?(s.kaspari)
I'm really not sold on using ExoPlayer -- what value does it give us? This is a *lot* of code...
Also, doesn't that use system libraries for audio/video decoding, meaning horrible things like libstagefright?
We want to leverage Exoplayer because of some reasons AFAICT.
- Reducing the implementation effort as possible.
- We want to avoid demuxing the M2TS packet ourselves due to legal concern.
- HLS is not only related to demuxing/decoding but also M3U8 parsing, packet downloading, bandwidth meter to switch the quality automatically and etc. which Exoplayer has already handled.


Compare with the apk size changing.
Before
48659354 Bytes fennec-54.0a1.en-US.android-arm-unsigned-unaligned.apk
48831619 Bytes fennec-54.0a1.en-US.android-arm.apk

After
49017848 Bytes fennec-54.0a1.en-US.android-arm-unsigned-unaligned.apk
49189225 Bytes fennec-54.0a1.en-US.android-arm.apk

350 KB increacing.

Hi nalexander,

I apply your patch and want to test if I can use the library built by ourself.

I encounter the "symbol cannot be resolved" error as below,

/mobile/android/base/java/org/mozilla/gecko/media/MediaDrmProxy.java:22: error: package com.google.android.exoplayer2.source.hls does not exist
 0:46.44 import com.google.android.exoplayer2.source.hls.HlsMediaSource;
 0:46.44                                       

/mobile/android/base/java/org/mozilla/gecko/media/MediaDrmProxy.java:51: error: cannot find symbol
 0:49.29       HlsMediaSource hlsMediaSrc = new HlsMediaSource(null,null,null,null);
 0:49.29       ^
 0:49.29   symbol:   class HlsMediaSource
 0:49.29   location: class MediaDrmProxy

It seems the library path is not set correctly that I cannot resolve the symbol.

I add the path into 

FENNEC_JARS = \
  exoplayer2.jar \
  gecko-browser.jar \
  gecko-thirdparty.jar \
  services.jar \
  ../javaaddons/javaaddons-1.0.jar \
  $(NULL)

as well but still not work.

Could you please guide me what I miss?

Thank you.
Flags: needinfo?(nalexander)
Hey, can we just download the ExoPlayer binary (jar) on demand? Then we don't have to build it and it also doesn't add to the APK size.
Flags: needinfo?(jacheng)
(In reply to James Cheng[:JamesCheng] from comment #20)
> We want to leverage Exoplayer because of some reasons AFAICT.
> - Reducing the implementation effort as possible.
> - We want to avoid demuxing the M2TS packet ourselves due to legal concern.
> - HLS is not only related to demuxing/decoding but also M3U8 parsing, packet
> downloading, bandwidth meter to switch the quality automatically and etc.
> which Exoplayer has already handled.

This sounds great on paper, but has this been vetted by the media and security teams? For instance, there are reasons we switched away from stagefright, and having it sneaking back in by using a third party player (assuming that's the case) doesn't really sounds appealing.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #21)
> Hey, can we just download the ExoPlayer binary (jar) on demand? Then we
> don't have to build it and it also doesn't add to the APK size.

Hi snorp,

I don't know the answer since I haven't think about we can on demand downloading jar and make program can use the jar in run-time.

Do you have any existing example in gecko that on demand using the jar?

If it is doable, I think we can adopt but it increases the implementation effort and complexity indeed.

Thank you.
Flags: needinfo?(jacheng)
(In reply to Mike Hommey [:glandium] from comment #22)
> (In reply to James Cheng[:JamesCheng] from comment #20)
> > We want to leverage Exoplayer because of some reasons AFAICT.
> > - Reducing the implementation effort as possible.
> > - We want to avoid demuxing the M2TS packet ourselves due to legal concern.
> > - HLS is not only related to demuxing/decoding but also M3U8 parsing, packet
> > downloading, bandwidth meter to switch the quality automatically and etc.
> > which Exoplayer has already handled.
> 
> This sounds great on paper, but has this been vetted by the media and
> security teams? For instance, there are reasons we switched away from
> stagefright, and having it sneaking back in by using a third party player
> (assuming that's the case) doesn't really sounds appealing.


To explain more, we tend to leverage only part of ExoPlayer, which are HLS manifest parser/TrackSelector/HTTP downloader/Bandwidth Meter/TS extractor.   We're not doing whole playback on it.

The progress we've got now is, we're able to get coded media frames (video/avc & audio/aac).
Then we'd like to create corresponding HLSMediaDecoder/HLSMediaDemuxer/HLSMediaReource wrapping it, just like the concept of MSE, so that HTMLMediaElement can interactive with it. By that, we can keep using Gecko's media pipeline.
MediaFormatReader is able to request MediaRawData from the HLSMediaResource.  After that, platform decoder module delivers these samples to remote decoder, which is now in a separated process.
Sorry for the noise, then.
(In reply to James Cheng[:JamesCheng] from comment #23)
> (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #21)
> > Hey, can we just download the ExoPlayer binary (jar) on demand? Then we
> > don't have to build it and it also doesn't add to the APK size.
> 
> Hi snorp,
> 
> I don't know the answer since I haven't think about we can on demand
> downloading jar and make program can use the jar in run-time.
> 
> Do you have any existing example in gecko that on demand using the jar?
> 
> If it is doable, I think we can adopt but it increases the implementation
> effort and complexity indeed.

Believe it or not, this is possible, but it's not easy to get right.  Android allows you to load dynamically load DEX files, and in fact Fennec has rudimentary support for doing this from Firefox Add-ons -- see the files linked in https://dxr.mozilla.org/mozilla-central/search?q=path%3Ajava&redirect=false.  (This is even tested, believe it or not.)

So you can't dynamically load the JAR file, but you can dynamically load DEX files, which is mostly equivalent.  From there, if you're very careful, you can inject the DEX code into your classloader.  I think you can even unload the DEX code, if you're clever -- that's what Android Studio's Instant Run is doing, IIUC.  I understand most of this well enough to give guidance.  However, I found that in practice it was very difficult to get right in the face of Proguard.  Bi-directional interfaces worked best when the interface classes were provided by the system or otherwise guaranteed to exist and be frozen, which made it difficult to de-couple Fennec from the add-on (equivalently, feature being provided).

However, it is possible, and there's a somewhat functional example in the tree -- see https://dxr.mozilla.org/mozilla-central/source/mobile/android/tests/javaaddons.
Flags: needinfo?(nalexander)
(In reply to James Cheng[:JamesCheng] from comment #20)
> We want to leverage Exoplayer because of some reasons AFAICT.

> I apply your patch and want to test if I can use the library built by
> ourself.

<snip>

> Could you please guide me what I miss?

Sure: around https://dxr.mozilla.org/mozilla-central/rev/b23d6277acca34a4b1be9a4c24efd3b999e47ec3/mobile/android/base/moz.build#882, you'll need to add "exoplayer2.jar".  I found this by looking for the "media/MediaDrmProxy.java" file you were modifying in `moz.build`, discovering it was in `gbjar` (which stands for "gecko-browser.jar"), and then knowing that we need to make "exoplayer2.jar" visible to "gecko-browser.jar".

Let me know if that's not enough.
Thanks you nalexander, it works.
Blocks: HLS_on_Fennec
Hi nalexander,

I met problem that may ask for your help first.

I want use Android Studio for Java code developing.

My goal is to let AndroidStudio that can index my Java code, not for using gradle build by itself.

So I just import the whole gecko but the index does not work well.

I have no idea so I want to try the instruction by [1]

My steps are

1. ./mach build (not sure if it is necessary to do before 2.)
2. ./gradlew clean app:assembleLocalOldDebug
and error occurred like [2]
the keyword "jumbo" makes me remind of the patch I provided for gradle build Exoplayer.

So I just add jumboMode = true in build.gradle as [3] said then do 2. again and it success.

I want to ask if it is a problem once we want to add Exoplayer into Gecko (need to add jumboMode=true wich I don't know the exact meaning and why it is related to the Exoplayer)

Another question is when I should do "./gradlew clean app:assembleLocalOldDebug" again? (once I do clobber?)


Thank you very much.


[1]
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build#Developing_Firefox_for_Android_in_Android_Studio_or_IDEA_IntelliJ

[2]
To run dex in process, the Gradle daemon needs a larger heap.
It currently has approximately 910 MB.
For faster builds, increase the maximum heap size for the Gradle daemon to more than 3072 MB.
To do this set org.gradle.jvmargs=-Xmx3072M in the project gradle.properties.
For more information see https://docs.gradle.org/current/userguide/build_environment.html
Dex: Error converting bytecode to dex:
Cause: com.android.dex.DexIndexOverflowException: Cannot merge new index 73311 into a non-jumbo instruction!
UNEXPECTED TOP-LEVEL EXCEPTION:
com.android.dex.DexIndexOverflowException: Cannot merge new index 73311 into a non-jumbo instruction!
at com.android.dx.merge.InstructionTransformer.jumboCheck(InstructionTransformer.java:111)
at com.android.dx.merge.InstructionTransformer.access$800(InstructionTransformer.java:26)
at com.android.dx.merge.InstructionTransformer$StringVisitor.visit(InstructionTransformer.java:74)
at com.android.dx.io.CodeReader.callVisit(CodeReader.java:114)
at com.android.dx.io.CodeReader.visitAll(CodeReader.java:89)
at com.android.dx.merge.InstructionTransformer.transform(InstructionTransformer.java:50)
at com.android.dx.merge.DexMerger.transformCode(DexMerger.java:826)
at com.android.dx.merge.DexMerger.transformMethods(DexMerger.java:800)
at com.android.dx.merge.DexMerger.transformClassData(DexMerger.java:773)
at com.android.dx.merge.DexMerger.transformClassDef(DexMerger.java:669)
at com.android.dx.merge.DexMerger.mergeClassDefs(DexMerger.java:523)
at com.android.dx.merge.DexMerger.mergeDexes(DexMerger.java:164)
at com.android.dx.merge.DexMerger.merge(DexMerger.java:188)
at com.android.dx.command.dexer.Main.mergeLibraryDexBuffers(Main.java:504)
at com.android.dx.command.dexer.Main.runMonoDex(Main.java:334)
at com.android.dx.command.dexer.Main.run(Main.java:277)
at com.android.dx.command.dexer.Main.main(Main.java:245)
at com.android.dx.command.Main.main(Main.java:106)

:app:transformClassesWithDexForLocalOldDebug FAILED

FAILURE: Build failed with an exception.

[3]
http://stackoverflow.com/questions/35806322/android-build-dex-jumbo-mode-ignored
Flags: needinfo?(nalexander)
In Addition,
I also met this error when I press "Run" button.

Error:(363, 1) A problem occurred configuring project ':app'.
> Task with name 'spoonLocalDebugAndroidTest' not found in project ':app'.

this is a known issue which I cannot find on bugzilla?

Thanks.
James: for Android Studio, you'll want to:
- |mach build| and |mach package|
- force Android Studio to build, using Build > Make Project
- reload the Gradle configuration using View > Tool Windows > Gradle and then clicking the little "sync" icon in the tool window
That should get the XML files into place and let the build and indexing continue.  https://bugzilla.mozilla.org/show_bug.cgi?id=1249421 tracks making this better but it's not a priority for me.

In Android Studio, make sure to choose the right target -- you probably want "local", not "localOld".  See MDN for what that means and how to choose the target.

The dex merging issue is a problem with the Android Gradle plugin.  The only thing I know is to |./mach gradle clean ...|.  I think it's the same as https://bugzilla.mozilla.org/show_bug.cgi?id=1247805.
Flags: needinfo?(nalexander)
Blocks: 1350241
Attachment #8856459 - Flags: review?(s.kaspari)
Attachment #8856460 - Flags: review?(s.kaspari)
Update Part3.

Since media related code was moved to GeckoView in Bug 1344347, so add exoplayer2 to gvjar.extra_jars for further usage.
James and I have finished the P.O.C about playing HLS content via HTMLMediaElement on Fennec, so I think it's time to make this bug continue in progress.

As Nick mentioned, there should be a follow-up to copy https://github.com/google/ExoPlayer/blob/25a966dc2300161448fa74dee5ee98322ff65604/library/proguard-rules.txt 
into tree and then make Fennec configuration to respect.
We would figure out how to do that.
Most of these patches were originally mine, but please flag me for final review before landing.  Thanks!
(In reply to Nick Alexander :nalexander from comment #37)
> Most of these patches were originally mine, but please flag me for final
> review before landing.  Thanks!

Thanks for the reminder, I only did a "hg import" from the commands on reviewboard, and modified part3 only, then a "hg push review", I didn't understand why Part1, Part2 were pushed again.  I was thinking that there should not be any redundant Part1 & Part2 because the MozReivew-CommitId are not changed, but I was wrong apparently :(.  I'll obsolete Part1/Part2 which were created by my push.
Attachment #8856459 - Attachment is obsolete: true
Attachment #8856460 - Attachment is obsolete: true
Hi Nick, I saw the commit message in your patch Part 3 ... " it should be guarded by a feature flag."
Do you mean a definition like MOZ_WEBRTC [1], so that we can choose to include or exclude exoplayer2 source code in mobile/android/base/moz.build [2] when building Fennec  ?

Or, we should follow what MOZ_NATIVE_DEVICES did, 
First, define it in confvar.sh, then configure related information in build.gradle [4], and also generate it for gradle build [5]. 

You want a flag that we could choose to include or exclude exoplayer2 source code during build time. 
I'm not sure if I understand it correctly and not sure about which is the correct way to do ...

Would you please elaborate more for us ?  Thanks :)

[1] http://searchfox.org/mozilla-central/rev/2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/old-configure.in#2704
[2] http://searchfox.org/mozilla-central/rev/2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/mobile/android/base/moz.build#200
[3] http://searchfox.org/mozilla-central/rev/2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/mobile/android/confvars.sh#39
[4] http://searchfox.org/mozilla-central/rev/2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/mobile/android/app/build.gradle#120
[5] http://searchfox.org/mozilla-central/rev/2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/mobile/android/base/generate_build_config.py#53
Flags: needinfo?(nalexander)
(In reply to Kilik Kuo [:kikuo] from comment #39)
> Hi Nick, I saw the commit message in your patch Part 3 ... " it should be
> guarded by a feature flag."

I've re-ordered your comments, so that my responses flow together.

> You want a flag that we could choose to include or exclude exoplayer2 source
> code during build time. 

Correct.

> I'm not sure if I understand it correctly and not sure about which is the
> correct way to do ...

I can help with this :)

> Do you mean a definition like MOZ_WEBRTC [1], so that we can choose to
> include or exclude exoplayer2 source code in mobile/android/base/moz.build
> [2] when building Fennec  ?
> 
> Or, we should follow what MOZ_NATIVE_DEVICES did, 
> First, define it in confvar.sh, then configure related information in
> build.gradle [4], and also generate it for gradle build [5]. 

These things are basically equivalent.  You shouldn't need to do any old-configure.in hacking, though.  Try to follow the example of MOZ_ANDROID_CUSTOM_TABS at https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/mobile/android/moz.configure#40.  So you'd add MOZ_ANDROID_EXOPLAYER2 (or maybe something more descriptive, like MOZ_ANDROID_HLS_SUPPORT) to moz.configure.  If you add it to the moz.build list around https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/mobile/android/base/moz.build#1239, you'll be able to use it in things that are preprocessed (like AndroidManifest.xml.in).  And finally, you can use it in the various build.gradle files without additional declaration, following https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/mobile/android/geckoview/build.gradle#46.  That is, you have access to `mozconfig.substs.MOZ_ANDROID_EXOPLAYER2` throughout the Gradle files.
 
> 
> Would you please elaborate more for us ?  Thanks :)

Hope that helps!

> [1]
> http://searchfox.org/mozilla-central/rev/
> 2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/old-configure.in#2704
> [2]
> http://searchfox.org/mozilla-central/rev/
> 2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/mobile/android/base/moz.build#200
> [3]
> http://searchfox.org/mozilla-central/rev/
> 2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/mobile/android/confvars.sh#39
> [4]
> http://searchfox.org/mozilla-central/rev/
> 2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/mobile/android/app/build.gradle#120
> [5]
> http://searchfox.org/mozilla-central/rev/
> 2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/mobile/android/base/
> generate_build_config.py#53
Flags: needinfo?(nalexander)
Attachment #8856461 - Attachment is obsolete: true
Attachment #8856461 - Attachment is obsolete: false
Comment on attachment 8856459 [details]
Bug 1341990 - Part 0: Add a feature flag to control source code build time coverage for ExoPlayer.

https://reviewboard.mozilla.org/r/128412/#review133640

Nick, Thanks for the detailed explanation, I've followed the instructions and created a # MOZ_ANDROID_HLS_SUPPORT, it works under folder mobile/.

Also I've modified a little bit, so that we can use this # for source code control under dom/media/, because the newly-created HLSDecoder/HLSDemuxer should also rely on this.  But I'm not sure if this is the best way.
Attachment #8856459 - Flags: review?(nalexander)
(In reply to Kilik Kuo [:kikuo] from comment #42)
> Comment on attachment 8856459 [details]
> Bug 1341990 - Part 4. Add a feature flag to control source cobuild time
> coverage.
> 
> https://reviewboard.mozilla.org/r/128412/#review133640
> 
> Nick, Thanks for the detailed explanation, I've followed the instructions
> and created a # MOZ_ANDROID_HLS_SUPPORT, it works under folder mobile/.
> 
> Also I've modified a little bit, so that we can use this # for source code
> control under dom/media/, because the newly-created HLSDecoder/HLSDemuxer
> should also rely on this.  But I'm not sure if this is the best way.

Should add, the reason I'm not using project_flag() is that a exception will be thrown, saying something like "The key already exists !".
(In reply to Kilik Kuo [:kikuo] from comment #43)
> (In reply to Kilik Kuo [:kikuo] from comment #42)
> > Comment on attachment 8856459 [details]
> > Bug 1341990 - Part 4. Add a feature flag to control source cobuild time
> > coverage.
> > 
> > https://reviewboard.mozilla.org/r/128412/#review133640
> > 
> > Nick, Thanks for the detailed explanation, I've followed the instructions
> > and created a # MOZ_ANDROID_HLS_SUPPORT, it works under folder mobile/.
> > 
> > Also I've modified a little bit, so that we can use this # for source code
> > control under dom/media/, because the newly-created HLSDecoder/HLSDemuxer
> > should also rely on this.  But I'm not sure if this is the best way.
> 
> Should add, the reason I'm not using project_flag() is that a exception will
> be thrown, saying something like "The key already exists !".

Hi, sorry for the delayed review.  I expect you to not need this; I'd like to see the code in dom/media/ and figure out how to avoid the old-configure.in hooks.
Comment on attachment 8856459 [details]
Bug 1341990 - Part 0: Add a feature flag to control source code build time coverage for ExoPlayer.

https://reviewboard.mozilla.org/r/128412/#review135056

In principle this is OK, but it should be done _before_ the other parts (as a Part 0, if you like) and the changes pushed into the other patches.  The commit message has a typo ("cobuild") that needs to be fixed.

I'm marking this r- 'cuz I want to see the rest of the integration into dom/media/.

::: old-configure.in:2866
(Diff revision 2)
>      fi
>  fi
>  
> +dnl ========================================================
> +dnl Define MOZ_ANDROID_HLS_SUPPORT symbol if HLS support is enabled
> +dnl on Fennec. We need this to include/exclude sort of gecko files.

Drop the "We need this...".
Attachment #8856459 - Flags: review?(nalexander) → review-
(In reply to Nick Alexander :nalexander from comment #45)
> Comment on attachment 8856459 [details]
> Bug 1341990 - Part 4. Add a feature flag to control source cobuild time
> coverage.
> 
> https://reviewboard.mozilla.org/r/128412/#review135056
> 
> In principle this is OK, but it should be done _before_ the other parts (as
> a Part 0, if you like) and the changes pushed into the other patches.  The
> commit message has a typo ("cobuild") that needs to be fixed.
> 
> I'm marking this r- 'cuz I want to see the rest of the integration into
> dom/media/.
> 
> ::: old-configure.in:2866
> (Diff revision 2)
> >      fi
> >  fi
> >  
> > +dnl ========================================================
> > +dnl Define MOZ_ANDROID_HLS_SUPPORT symbol if HLS support is enabled
> > +dnl on Fennec. We need this to include/exclude sort of gecko files.
> 
> Drop the "We need this...".

Thanks for the feedback.
The rest of codes are still WIP, mainly we demonstrated how the # MOZ_ANADROID_HLS_SUPPORT is used in this commit [1].
Please have a look and any suggestion will be appreciated : )

A thought just came up to my mind, I may not need to modify old-configure.in in this bug as those implementation will be included in other bugs, i.e. Bug 1345752.  I could make it in other bugs.  In this bug, I should focus on constrain the area which will be affected by ExoPlayer source code, does that make sense to you ?

[1] https://github.com/JamesWCCheng/gecko-dev/commit/befdb6ee0ccaa234e566d22b09528725ba933fec
Flags: needinfo?(nalexander)
(In reply to Kilik Kuo [:kikuo] from comment #46)
> (In reply to Nick Alexander :nalexander from comment #45)
> > Comment on attachment 8856459 [details]
> > Bug 1341990 - Part 4. Add a feature flag to control source cobuild time
> > coverage.
> > 
> > https://reviewboard.mozilla.org/r/128412/#review135056
> > 
> > In principle this is OK, but it should be done _before_ the other parts (as
> > a Part 0, if you like) and the changes pushed into the other patches.  The
> > commit message has a typo ("cobuild") that needs to be fixed.
> > 
> > I'm marking this r- 'cuz I want to see the rest of the integration into
> > dom/media/.
> > 
> > ::: old-configure.in:2866
> > (Diff revision 2)
> > >      fi
> > >  fi
> > >  
> > > +dnl ========================================================
> > > +dnl Define MOZ_ANDROID_HLS_SUPPORT symbol if HLS support is enabled
> > > +dnl on Fennec. We need this to include/exclude sort of gecko files.
> > 
> > Drop the "We need this...".
> 
> Thanks for the feedback.
> The rest of codes are still WIP, mainly we demonstrated how the #
> MOZ_ANADROID_HLS_SUPPORT is used in this commit [1].
> Please have a look and any suggestion will be appreciated : )
> 
> A thought just came up to my mind, I may not need to modify old-configure.in
> in this bug as those implementation will be included in other bugs, i.e. Bug
> 1345752.  I could make it in other bugs.  In this bug, I should focus on
> constrain the area which will be affected by ExoPlayer source code, does
> that make sense to you ?
> 
> [1]
> https://github.com/JamesWCCheng/gecko-dev/commit/
> befdb6ee0ccaa234e566d22b09528725ba933fec

I'm glad I looked at this.  You shouldn't need the old-configure.in stuff at all: you just need to set the local DEFINES in dom/media/moz.build (and any other directory you use the flag in C++ code), like at https://dxr.mozilla.org/mozilla-central/rev/8b854986038cf3f3f240697e27ef48ea65914c13/dom/media/moz.build#320.
Flags: needinfo?(nalexander)
Hi Nick,

Thanks for the tips provided in Comment 47 :). That looks much nicer !

Now we met the final (I hope) problem here.
As you mentioned in Part1 that we need to follow the ProGuard rule for ExoPlayer [1]. I'm trying to do so in the attached patch. But I found that classes in "obj-dir/mobile/android/base/jars-proguarded/exoplayer2.jar" are NOT obfuscated at all even I applied either one of the following two commands.

-keep,allowobfuscation class com.google.android.exoplayer2.**
-keeppackagenames !com.google.android.exoplayer2

Since ExoPlayer is an open source project and there are only 2 rules in its proguard-rules.txt, I'm wondering that do we need to obfuscate ExoPlayer source codes as many as possible ?  If so, could you guide us how to achieve that through gecko's configuration ?  Thanks.

[1] https://github.com/google/ExoPlayer/blob/25a966dc2300161448fa74dee5ee98322ff65604/library/proguard-rules.txt
Attachment #8863706 - Flags: feedback?(nalexander)
(In reply to Kilik Kuo [:kikuo] from comment #48)
> Created attachment 8863706 [details] [diff] [review]
> 0001-Test-ProGuard-for-exoplayer.patch

Additionally, this was tested in non-debug build.
Comment on attachment 8863706 [details] [diff] [review]
0001-Test-ProGuard-for-exoplayer.patch

Review of attachment 8863706 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Kilik!  This looks great.  Proguard does many things, but the big two are obfuscation and dead code elimination.  Obfuscation is about saving space: Java stores long string names like "org.mozilla.gecko.BrowserApp" that obfuscation can shorten to "o.m.g.BrowserApp".  Dead code elimination strips methods and classes that aren't referenced.  Of course, not all things can be obfuscated or eliminated: the Android system needs to have certain names match the AndroidManifest.xml and needs to have certain classes available.

The rule that "doesn't work" for you,

-keep,allowobfuscation class com.google.android.exoplayer2.** 

says that all the classes need to be kept (they are referenced) but the names can change.  The other one,

-keeppackagenames !com.google.android.exoplayer2

should "just work".

Now, _Fennec does not obfuscate at all_.  This was a choice made long ago when we used a lot of reflection (and before we really understood Proguard).  So you should be able to keep the "allowobfuscation" (and perhaps note that it won't do anything), in case we ever _start_ obfuscating.  And the -keep and -keeppackagenames should make sure Fennec doesn't get rid of the "unreferenced" classes.

Does that clarify?  Can you update the comments in the patch, and then I can test locally to verify that everything is working as I expect?
Attachment #8863706 - Flags: feedback?(nalexander) → feedback+
Thanks Nick, the explanation indeed casts light on understanding fennec's build procedure.  New patch is attached.

You mentioned that _fennec_doesnt_obfuscate_at_all_ [1] and further more fennec "-keep" lots of things...

[1] http://searchfox.org/mozilla-central/rev/ae8c2e2354db652950fe0ec16983360c21857f2a/mobile/android/config/proguard/proguard.cfg#166

Originally, I expected

-keeppackagenames !com.google.android.exoplayer2

would respect the command '-dontobfuscate' but still obfuscate everything in the exoplayer package.  Then I checked the size of exoplayer2.jar generated in different phases ...

1) obj-dir/mobile/android/base/exoplayer2.jar  ==> 980.3 kB
2) obj-dir/mobile/android/base/jars-proguradedexoplayer2.jar ==> 909.6 kb

Then I analyzed the PATH/jars-proguarded/exoplayer2.jar via JD-GUI (java decompiler), some unreferenced "import" statements were removed and this should be the result of dead code elimination.  But I found nothing was obfuscated.
Is this expected ?  Does '-dontobfuscate' suppress other commands ?

And that's the reason I put the comment "#Doesn't work" in previous Attachment 8863706 [details] [diff].
Attachment #8863706 - Attachment is obsolete: true
Attachment #8864027 - Flags: feedback?(nalexander)
Comment on attachment 8864027 [details] [diff] [review]
0001-Respect-the-ProGuard-rules-for-ExoPlayer.patch

Review of attachment 8864027 [details] [diff] [review]:
-----------------------------------------------------------------

I think this does what you want, but I'd roll back partially and update the comments.  I think it is working (although I haven't looked at the JAR outputs) because nothing should be getting obfuscated.

As for not much code being eliminated: I don't know enough about the library to understand why that is the case.

::: mobile/android/config/proguard/exoplayer-keeps.cfg
@@ +1,2 @@
> +# ExoPlayer
> +# via https://github.com/google/ExoPlayer/blob/25a966dc2300161448fa74dee5ee98322ff65604/library/proguard-rules.txt

This seems like a very old version -- January 30, 2017.  The library layout seems to have changed a lot.  Are you aware of this?

@@ +1,4 @@
> +# ExoPlayer
> +# via https://github.com/google/ExoPlayer/blob/25a966dc2300161448fa74dee5ee98322ff65604/library/proguard-rules.txt
> +
> +# Obfuscate anything which is not referenced by other rules in com.google.android.exoplayer2

Fennec doesn't obfuscate now, and probably won't anytime soon, so it's probably best to keep the version from your earlier patch:

+# Doesn't work
+# -keep,allowobfuscation class com.google.android.exoplayer2.**
+# Doesn't work
+# -keeppackagenames !com.google.android.exoplayer2

That is, if we _eventually_ obfuscate, then we're allowed to obfuscate these classes, but we need to keep package names of these things, etc, etc.

As it is, I don't think Proguard turns on obfuscation when it sees allowobfuscation.  So this should be fine.
Attachment #8864027 - Flags: feedback?(nalexander) → review+
(In reply to Nick Alexander :nalexander from comment #52)
> Comment on attachment 8864027 [details] [diff] [review]
> 0001-Respect-the-ProGuard-rules-for-ExoPlayer.patch
> 
> Review of attachment 8864027 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this does what you want, but I'd roll back partially and update the
> comments.  I think it is working (although I haven't looked at the JAR
> outputs) because nothing should be getting obfuscated.
> 
> As for not much code being eliminated: I don't know enough about the library
> to understand why that is the case.
> 
> ::: mobile/android/config/proguard/exoplayer-keeps.cfg
> @@ +1,2 @@
> > +# ExoPlayer
> > +# via https://github.com/google/ExoPlayer/blob/25a966dc2300161448fa74dee5ee98322ff65604/library/proguard-rules.txt
> 
> This seems like a very old version -- January 30, 2017.  The library layout
> seems to have changed a lot.  Are you aware of this?
> 

Though I know the ExoPlayer repository changes actively while we're developing our customized components and integrating to Gecko, we've decided to freeze the code for development stability first.

After I looked into the library layout and the release notes r2.3.x/r2.4.0 (the source code attached are in r2.2.0), some improvements have been done for HLS and it seems easier to eliminate the code which we don't need !

> @@ +1,4 @@
> > +# ExoPlayer
> > +# via https://github.com/google/ExoPlayer/blob/25a966dc2300161448fa74dee5ee98322ff65604/library/proguard-rules.txt
> > +
> > +# Obfuscate anything which is not referenced by other rules in com.google.android.exoplayer2
> 
> Fennec doesn't obfuscate now, and probably won't anytime soon, so it's
> probably best to keep the version from your earlier patch:
> 
> +# Doesn't work
> +# -keep,allowobfuscation class com.google.android.exoplayer2.**
> +# Doesn't work
> +# -keeppackagenames !com.google.android.exoplayer2
> 
> That is, if we _eventually_ obfuscate, then we're allowed to obfuscate these
> classes, but we need to keep package names of these things, etc, etc.
> 

Understood !

> As it is, I don't think Proguard turns on obfuscation when it sees
> allowobfuscation.  So this should be fine.

Thanks for the explanation. I checked the latest version (r2.4.0), surprisingly only extension components are proguarded.
I think it should also be fine now for things which we want to put into Fennec from ExoPlayer to apply Gecko's proguard rules.
I'd try to introduce the version r.2.4.0 and clean up old patches then provide new ones for review.  
Thanks for the reminder.
Attachment #8840319 - Attachment is obsolete: true
Attachment #8856461 - Attachment is obsolete: true
Attachment #8856461 - Attachment is patch: true
Attachment #8856461 - Attachment mime type: text/x-review-board-request → text/plain
Comment on attachment 8856459 [details]
Bug 1341990 - Part 0: Add a feature flag to control source code build time coverage for ExoPlayer.

https://reviewboard.mozilla.org/r/128410/#review140924

Hi Nick,
I've updated the source from ExoPlayer r2.2.0 to r2.4.0 and would like to stick to r.2.4.0 for now. 
Meanwihle I found there's no proguard files for ExoPlayer library (proguard-rules.txt is only used for extension components), therefore I don't think we need to provide additional rules. Fennec's original rules should be fine.

Please do me a favor to review these patches. Thanks!
Assignee: nobody → kikuo
(In reply to Kilik Kuo [:kikuo] from comment #58)
> Comment on attachment 8856459 [details]
> Bug 1341990 - Part 0: Add a feature flag to control source code build time
> coverage for ExoPlayer.
> 
> https://reviewboard.mozilla.org/r/128410/#review140924
> 
> Hi Nick,
> I've updated the source from ExoPlayer r2.2.0 to r2.4.0 and would like to
> stick to r.2.4.0 for now. 
> Meanwihle I found there's no proguard files for ExoPlayer library
> (proguard-rules.txt is only used for extension components), therefore I
> don't think we need to provide additional rules. Fennec's original rules
> should be fine.
> 
> Please do me a favor to review these patches. Thanks!

Kilik, I'll get to these on Monday -- I'm swamped today.  Sorry for the delayed review!
Comment on attachment 8856459 [details]
Bug 1341990 - Part 0: Add a feature flag to control source code build time coverage for ExoPlayer.

https://reviewboard.mozilla.org/r/128412/#review143120

::: commit-message-73163:1
(Diff revision 3)
> +Bug 1341990 - Part 0: Add a feature flag to control source code build time coverage for ExoPlayer.

nit: Part 0: Add MOZ_ANDROID_HLS_SUPPORT build flag controlling HTTP Live Streaming support in Fennec.

I want the flag name and the acronym expansion in the commit message.

::: mobile/android/moz.configure:52
(Diff revision 3)
>  project_flag('MOZ_SWITCHBOARD',
>               help='Include Switchboard A/B framework on Android',
>               default=True)
>  
> +project_flag('MOZ_ANDROID_HLS_SUPPORT',
> +             help='Enable HLS support for Android via ExoPlayer r2.4.0',

nit: let's not bake the version into the string.  How about:

`help='Enable HLS (HTTP Live Streaming) support (currently using the ExoPlayer library)'`

Then we have the acronym expansion and the reference to the underlying technology, but if we change the technology we don't need to rename the flag.
Attachment #8856459 - Flags: review?(nalexander) → review+
Comment on attachment 8866171 [details]
Bug 1341990 - Part 1: Import ExoPlayer sources (r2.4.0) without {ui/,dash/,smoothstreaming/,all/} subdirectories.

https://reviewboard.mozilla.org/r/137778/#review143124

Tiny nit below applies to the commit message topline comment as well.

I didn't look at the imported source at all.

::: commit-message-459ee:9
(Diff revision 1)
> +
> +ExoPlayer is licensed Apache 2.0, so it's fully compatible with
> +Mozilla's MPL.
> +
> +The import is the contents of https://github.com/google/ExoPlayer/tree/d979469659861f7fe1d39d153b90bdff1ab479cc/library,
> +minus the ui/dash/smoothstreaming/all directories.

tiny nit: this looks like a single path.  How about "without the {ui/,dash/,smoothstreaming/,all/} subdirectories."?
Attachment #8866171 - Flags: review?(nalexander) → review+
Comment on attachment 8866172 [details]
Bug 1341990 - Part 2: Make ExoPlayer (r2.4.0) build against Android SDK 23.

https://reviewboard.mozilla.org/r/137780/#review143130

If it works for you, it works for me.

Good job with the commit message -- my edits were just polishing the grammar but you clearly conveyed what you were doing and why.  That makes it much easier to review, thank you!

::: commit-message-bf381:7
(Diff revision 1)
> +
> +Right now, Fennec builds against Android SDK 23.  ExoPlayer expects to
> +build against Android SDK 24 (but targets Android platform 9 and
> +above).
> +
> +By replacing the definition with constant values which are defined on

nit: "We replace constant values introduced in Android SDK 24 with values manually copied from  https://developer.android.com/index.html and we cull unused code from CryptoInfo.java.  Together, these changes allow ExoPlayer to build against Android SDK 23."

::: commit-message-bf381:12
(Diff revision 1)
> +By replacing the definition with constant values which are defined on
> +the site, https://developer.android.com/index.html, and additionaly
> +commenting out part of unused code in CryptoInfo.java, that would
> +allow ExoPlayer to build against Android SDK 23.
> +
> +A follow-up should be created once Fennec is goning to build against

File this follow-up, and change this line to something like: "Bug XXX tracks reverting these changes once Bug 1259098 lands and Fennec builds against Android SDK 24."

::: mobile/android/thirdparty/com/google/android/exoplayer2/C.java:115
(Diff revision 1)
>    public static final int CRYPTO_MODE_AES_CTR = MediaCodec.CRYPTO_MODE_AES_CTR;
>    /**
>     * @see MediaCodec#CRYPTO_MODE_AES_CBC
>     */
>    @SuppressWarnings("InlinedApi")
> -  public static final int CRYPTO_MODE_AES_CBC = MediaCodec.CRYPTO_MODE_AES_CBC;
> +  public static final int CRYPTO_MODE_AES_CBC = 0x2;

Consider adding an `ConstantsFromAndroidV24` class or similar that has exactly the same names as the constants you're importing, so that reverting is just s/ConstantsFromAndroidV24/MediaFormat/ in each place.

This is up to you, since you're the one (or the team!) that will want to search for these constants, replace them, revert the changes, etc.  I see that the constant name is in _this_ source file, so you're most of the way to finding usages easily already.

::: mobile/android/thirdparty/com/google/android/exoplayer2/Format.java:602
(Diff revision 1)
>        return;
>      }
> -    maybeSetIntegerV16(format, MediaFormat.KEY_COLOR_TRANSFER, colorInfo.colorTransfer);
> -    maybeSetIntegerV16(format, MediaFormat.KEY_COLOR_STANDARD, colorInfo.colorSpace);
> -    maybeSetIntegerV16(format, MediaFormat.KEY_COLOR_RANGE, colorInfo.colorRange);
> -    maybeSetByteBufferV16(format, MediaFormat.KEY_HDR_STATIC_INFO, colorInfo.hdrStaticInfo);
> +    maybeSetIntegerV16(format, "color-transfer", colorInfo.colorTransfer);
> +    maybeSetIntegerV16(format, "color-standard", colorInfo.colorSpace);
> +    maybeSetIntegerV16(format, "color-range", colorInfo.colorRange);
> +    maybeSetByteBufferV16(format,"hdr-static-info", colorInfo.hdrStaticInfo);

nit: space after ,.

::: mobile/android/thirdparty/com/google/android/exoplayer2/decoder/CryptoInfo.java:134
(Diff revision 1)
>    @TargetApi(24)
>    private static final class PatternHolderV24 {
>  
>      private final android.media.MediaCodec.CryptoInfo frameworkCryptoInfo;
> -    private final android.media.MediaCodec.CryptoInfo.Pattern pattern;
> +    /**
> +     * NOTE : Mozilla Fennec is built against SDK v23 now, in the meanwhile this class

No need for the NOTE: and indentation; you can just comment.  Reference the two tickets (follow-up to revert these changees and the Android N build bug) in the comment, please.
Attachment #8866172 - Flags: review?(nalexander) → review+
Comment on attachment 8866173 [details]
Bug 1341990 - Part 3: Include ExoPlayer in Firefox for Android builds.

https://reviewboard.mozilla.org/r/137782/#review143134

This looks good to me.  If it's green on try (android-api-15, android-api-15-gradle), then it's good from me!

Sorry for the delayed review, especially when you had this all polished up for me.

The only things remaining are to get the final APK size increases reported on the ticket, and to get sebastian and snorp to sign off that they're okay with the APK bloat.  Great work!
Attachment #8866173 - Flags: review?(nalexander) → review+
(In reply to Nick Alexander :nalexander from comment #63)
> Comment on attachment 8866173 [details]
> Bug 1341990 - Part 3: Include ExoPlayer in Firefox for Android builds.
> 
> https://reviewboard.mozilla.org/r/137782/#review143134
> 
> This looks good to me.  If it's green on try (android-api-15,
> android-api-15-gradle), then it's good from me!
> 
> Sorry for the delayed review, especially when you had this all polished up
> for me.
> 
> The only things remaining are to get the final APK size increases reported
> on the ticket, and to get sebastian and snorp to sign off that they're okay
> with the APK bloat.  Great work!

Thank you for the guidance and review : )
I'll address these issues and provide required information for further actions.
Comment on attachment 8866172 [details]
Bug 1341990 - Part 2: Make ExoPlayer (r2.4.0) build against Android SDK 23.

https://reviewboard.mozilla.org/r/137780/#review143130

> Consider adding an `ConstantsFromAndroidV24` class or similar that has exactly the same names as the constants you're importing, so that reverting is just s/ConstantsFromAndroidV24/MediaFormat/ in each place.
> 
> This is up to you, since you're the one (or the team!) that will want to search for these constants, replace them, revert the changes, etc.  I see that the constant name is in _this_ source file, so you're most of the way to finding usages easily already.

Thanks for the suggestion, it sounds good but that would require an extra file.
Because there are not that many places to revert. I'd leave it as it is.
Try result looks good. https://treeherder.mozilla.org/#/jobs?repo=try&revision=7766ea443ef6 (The busted one is not related to these patches)

The following size information are based on local NON-DEBUG build.

Without patches
fennec-55.0a1.en-US.android-arm.apk / 32.5 MB (3,247,5605 bytes)
==============================================================
With patches
fennec-55.0a1.en-US.android-arm.apk / 32.5 MB (3,250,3891 bytes)

The APK size increased around 27.6 KB.
Hi sebastian, snorp, is the APK bloat acceptable ? Or do you need other information, please let me know. Thanks.
Flags: needinfo?(snorp)
Flags: needinfo?(s.kaspari)
(In reply to Kilik Kuo [:kikuo] from comment #70)
> Try result looks good.
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=7766ea443ef6 (The
> busted one is not related to these patches)
> 
> The following size information are based on local NON-DEBUG build.
> 
> Without patches
> fennec-55.0a1.en-US.android-arm.apk / 32.5 MB (3,247,5605 bytes)
> ==============================================================
> With patches
> fennec-55.0a1.en-US.android-arm.apk / 32.5 MB (3,250,3891 bytes)
> 
> The APK size increased around 27.6 KB.

Isn't it only around 3kb?  That's... much less than I expected.

From https://treeherder.mozilla.org/#/jobs?repo=try&revision=7766ea443ef6&selectedJob=99737896:

Size of classes.dex: 7188404 bytes
Size of libxul.so: 21864060 bytes
Size of omni.ja: 6105773 bytes
Size of target.apk: 35717892 bytes

From https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=85e5d15c31691c89b82d6068c26260416493071f&selectedJob=99797276:

Size of classes.dex: 6478160 bytes
Size of libxul.so: 22040336 bytes
Size of omni.ja: 6099743 bytes
Size of target.apk: 35574020 bytes 

It's not a perfect comparison, but we can see the "real" classes.dex bloat is about 719 - 648 = ~70kB, and the APK bloat is about 35718 - 35574 = ~144kB.  That seems more realistic, and (to my eye) appropriate for the size of the feature we're implementing.
Yeah, I think this is probably acceptable.
Flags: needinfo?(snorp)
(In reply to Nick Alexander :nalexander from comment #71)
> (In reply to Kilik Kuo [:kikuo] from comment #70)
> > Try result looks good.
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=7766ea443ef6 (The
> > busted one is not related to these patches)
> > 
> > The following size information are based on local NON-DEBUG build.
> > 
> > Without patches
> > fennec-55.0a1.en-US.android-arm.apk / 32.5 MB (3,247,5605 bytes)
> > ==============================================================
> > With patches
> > fennec-55.0a1.en-US.android-arm.apk / 32.5 MB (3,250,3891 bytes)
> > 
> > The APK size increased around 27.6 KB.
> 
> Isn't it only around 3kb?  That's... much less than I expected.
> 
> From
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=7766ea443ef6&selectedJob=99737896:
> 
> Size of classes.dex: 7188404 bytes
> Size of libxul.so: 21864060 bytes
> Size of omni.ja: 6105773 bytes
> Size of target.apk: 35717892 bytes
> 
> From
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> central&revision=85e5d15c31691c89b82d6068c26260416493071f&selectedJob=9979727
> 6:
> 
> Size of classes.dex: 6478160 bytes
> Size of libxul.so: 22040336 bytes
> Size of omni.ja: 6099743 bytes
> Size of target.apk: 35574020 bytes 
> 
> It's not a perfect comparison, but we can see the "real" classes.dex bloat
> is about 719 - 648 = ~70kB, and the APK bloat is about 35718 - 35574 =
> ~144kB.  That seems more realistic, and (to my eye) appropriate for the size
> of the feature we're implementing.

Cool, Nick, thanks for the information.
I didn't realize that we've size log from treeherder's log. Learned!
Attachment #8841014 - Attachment is obsolete: true
Attachment #8841015 - Attachment is obsolete: true
Attachment #8841016 - Attachment is obsolete: true
Comment on attachment 8869529 [details]
Bug 1341990 - Post: Update build system to reference ExoPlayer in GeckoView thirdparty source directory.

https://reviewboard.mozilla.org/r/141118/#review144806

::: mobile/android/geckoview/build.gradle:74
(Diff revision 1)
>      }
>  
>      sourceSets {
>          main {
>              java {
>                  srcDir "${topsrcdir}/mobile/android/geckoview/src/thirdparty/java"

Nick, thank you for the explanation.
So IIUC, the point is we have put ExoPlayer source code to the Fennec-denpendent thirdparty folder, that's why the source code is not included becuase of this line of code while building with gradle, is that right ?

I could havd been aware of that as I noticed that most of media files were moved to the folder under geckoview/scr/main/, but sadly I didn't ask.

It's very nice of you to help us having a more clear picture between moz.build and build.gradle system. Thanks again.
Comment on attachment 8869529 [details]
Bug 1341990 - Post: Update build system to reference ExoPlayer in GeckoView thirdparty source directory.

https://reviewboard.mozilla.org/r/141118/#review144806

> Nick, thank you for the explanation.
> So IIUC, the point is we have put ExoPlayer source code to the Fennec-denpendent thirdparty folder, that's why the source code is not included becuase of this line of code while building with gradle, is that right ?
> 
> I could havd been aware of that as I noticed that most of media files were moved to the folder under geckoview/scr/main/, but sadly I didn't ask.
> 
> It's very nice of you to help us having a more clear picture between moz.build and build.gradle system. Thanks again.

Should add, the comment above is the reply to Bug 1350241 Comment 46
Comment on attachment 8869528 [details]
Bug 1341990 - Post: Move ExoPlayer to GeckoView thirdparty source directory.

https://reviewboard.mozilla.org/r/141116/#review145048

LGTM
Attachment #8869528 - Flags: review?(max) → review+
Comment on attachment 8869529 [details]
Bug 1341990 - Post: Update build system to reference ExoPlayer in GeckoView thirdparty source directory.

https://reviewboard.mozilla.org/r/141118/#review145042

LGTM
Attachment #8869529 - Flags: review?(max) → review+
Comment on attachment 8856459 [details]
Bug 1341990 - Part 0: Add a feature flag to control source code build time coverage for ExoPlayer.

https://reviewboard.mozilla.org/r/128410/#review145250

Fold post patches into Part2 & Part 3.
Modify commit messages and put all reviewers for detail information.
Comment on attachment 8856459 [details]
Bug 1341990 - Part 0: Add a feature flag to control source code build time coverage for ExoPlayer.

https://reviewboard.mozilla.org/r/128410/#review145396

Per discussion with maliu offline, I decided to make the patch apply history more simple and that would be eaiser to revert once Fennec builds against 24.
So I directly imported ExoPlayer source code into correct thirdparty folder, that is, mobile/android/geckoview/src/thirdparty/.

Thanks for the instructions.
Comment on attachment 8866172 [details]
Bug 1341990 - Part 2: Make ExoPlayer (r2.4.0) build against Android SDK 23.

https://reviewboard.mozilla.org/r/137780/#review145402

Sorry for span, remove unrelated commit message.
The size before & after including ExoPlayer source now is

Before
https://treeherder.mozilla.org/#/jobs?repo=try&tochange=e7e4291582f9&fromchange=335f2024b9106761802208b2e360f7d962c1c747&selectedJob=101154507

[task 2017-05-23T08:11:51.979627Z] 08:11:51     INFO - TinderboxPrint: Size of target.apk<br/>35557387 bytes
[task 2017-05-23T08:11:51.993672Z] 08:11:51     INFO - TinderboxPrint: Size of libxul.so<br/>22028712 bytes
[task 2017-05-23T08:11:51.993863Z] 08:11:51     INFO - TinderboxPrint: Size of classes.dex<br/>6476824 bytes
[task 2017-05-23T08:11:51.994041Z] 08:11:51     INFO - TinderboxPrint: Size of omni.ja<br/>6094927 bytes

After
https://treeherder.mozilla.org/#/jobs?repo=try&tochange=e7e4291582f9&fromchange=335f2024b9106761802208b2e360f7d962c1c747&selectedJob=101157426

[task 2017-05-23T08:17:42.271095Z] 08:17:42     INFO - TinderboxPrint: Size of target.apk<br/>35715418 bytes
[task 2017-05-23T08:17:42.285354Z] 08:17:42     INFO - TinderboxPrint: Size of libxul.so<br/>21861576 bytes
[task 2017-05-23T08:17:42.285545Z] 08:17:42     INFO - TinderboxPrint: Size of classes.dex<br/>7188404 bytes
[task 2017-05-23T08:17:42.285725Z] 08:17:42     INFO - TinderboxPrint: Size of omni.ja<br/>6105773 bytes


The classes.dex bloat is about 7188-6477 = ~691 KB.
The apk bloat is about 35715-35557 = ~158 KB.
We just talked on IRC. 158kB in APK size sounds fine to me if we get significant improvements from that.
Flags: needinfo?(s.kaspari)
Pushed by kikuo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9b0805d8354b
Part 0: Add a feature flag to control source code build time coverage for ExoPlayer. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/4bac6c0c03df
Part 1: Import ExoPlayer sources (r2.4.0) without {ui/,dash/,smoothstreaming/,all/} subdirectories. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/d615a9663923
Part 2: Make ExoPlayer (r2.4.0) build against Android SDK 23. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/5b79baafde67
Part 3: Include ExoPlayer in Firefox for Android builds. r=nalexander
Hi sheriff,

I have no idea what's the conditional difference between 

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5b79baafde67e59fb9e9f07af3dc8843475fe142&selectedJob=101225377   (bad)

and 

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=438107a780653172b70b1fbd89fa2cc76f458fb4  (looks good)

we manually pushed try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2eb41fe032543a85bd838c77c8133a4be87da682  (looks good too)

Could you please let us know what's the different between those try results, is there any build configuration differences?

Currently I have no idea why my try looks good but autoland looks bad.

Thank you.
Flags: needinfo?(ryanvm)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #97)
> Backed out for breaking a number of Android jobs.
> https://hg.mozilla.org/integration/autoland/rev/
> 438107a780653172b70b1fbd89fa2cc76f458fb4
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=101225367&repo=autoland
> https://treeherder.mozilla.org/logviewer.html#?job_id=101225383&repo=autoland
> https://treeherder.mozilla.org/logviewer.html#?job_id=101225379&repo=autoland
> https://treeherder.mozilla.org/logviewer.html#?job_id=101225363&repo=autoland
> 
> etc

Oops my bad, code base is too old, it seems "delayed_getattr" not existing anymore.
I'll rebase it agin.  Thanks : )
Flags: needinfo?(ryanvm)
Comment on attachment 8856459 [details]
Bug 1341990 - Part 0: Add a feature flag to control source code build time coverage for ExoPlayer.

https://reviewboard.mozilla.org/r/128410/#review145642

rebase to https://hg.mozilla.org/try/rev/6dfa56094f0c
Pushed by kikuo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/76009b68fbe3
Part 0: Add a feature flag to control source code build time coverage for ExoPlayer. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/edca036e4f97
Part 1: Import ExoPlayer sources (r2.4.0) without {ui/,dash/,smoothstreaming/,all/} subdirectories. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/2bc81147c439
Part 2: Make ExoPlayer (r2.4.0) build against Android SDK 23. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/dcc4edc6a896
Part 3: Include ExoPlayer in Firefox for Android builds. r=nalexander
As expected this change increased the apk size a bunch:

== Change summary for alert #6814 (as of May 24 2017 02:29 UTC) ==

Regressions:

  1%  installer size summary android-api-15-gradle opt      35,878,783.92 -> 36,216,140.08
  1%  installer size summary android-4-2-x86 opt            35,557,715.42 -> 35,893,322.58
  1%  installer size summary android-4-0-armv7-api15 opt    35,607,192.42 -> 35,939,063.33

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=6814
Depends on: 1367768
Depends on: 1370154
Depends on: 1370179
FYI, Coverity found a bunch of defects in this component.
The defect density is pretty high (1.13)
many "Dereference after null check", "Explicit null dereferenced" or "Unguarded read"/"Unguarded write"

More here:
https://scan.coverity.com/projects/firefox-mobile
(I am the admin, I can approve people)
(In reply to Sylvestre Ledru [:sylvestre] from comment #109)
> FYI, Coverity found a bunch of defects in this component.
> The defect density is pretty high (1.13)
> many "Dereference after null check", "Explicit null dereferenced" or
> "Unguarded read"/"Unguarded write"
> 
> More here:
> https://scan.coverity.com/projects/firefox-mobile
> (I am the admin, I can approve people)

Thanks for the information, I've signed in the site, and will take some time to check if we need to fix these defects. Bug 1371247.
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 55 → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: