Closed Bug 1368954 Opened 7 years ago Closed 7 years ago

[Fennec][HLS] Separate source code which will affect generated JNI wrappers to avoid build bustage.

Categories

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

enhancement
Not set
normal

Tracking

(firefox56 fixed)

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: kikuo, Assigned: kikuo)

References

Details

Attachments

(2 files)

Arises from Bug 1368925.

At first, we want source code related to ExoPlayer included in nightly only until the feature is well tested. Then we'll make it get into the train.

But the generated JNI wrappers from our glue code will make beta build bustage.

I'll try leverage Java Reflection to create a java wrapper which doesn't make use of the class symbol of GeckoHlsPlayer to avoid the build bustage.
We have some @WrapForJNI java codes which is built only on "nightly".

But the GeneratedJNI* related codes are generated without "nightly only".

It will make http://searchfox.org/mozilla-central/rev/b318c7dca7392bd16c0b11929f55b1be133f0b31/mobile/android/base/Makefile.in#541

be true then get the build error....

The only solution we can figure out is to make @WrapForJNI methods decouple from "nightly only" code by using java reflection.

But reflection seems not a good way to deal with this case.

Hi Jim,

Do you have any idea how to deal with this situation when some @WrapForJNI methods is declared when XXX only? Or reflection is the only way to solve this?

Thanks.
Flags: needinfo?(nchen)
See bug 1368925. Currently there is no way to make JNI bindings conditional. But for the next cycle, we can probably add preprocessing support for JNI bindings.
Flags: needinfo?(nchen)
(In reply to Jim Chen [:jchen] [:darchons] from comment #2)
> See bug 1368925. Currently there is no way to make JNI bindings conditional.
> But for the next cycle, we can probably add preprocessing support for JNI
> bindings.

I don't think this will go far enough.  The annotation processor works by extracting annotations from the compiled JAR files.  If MOZ_FOO is disabled, there will be no reference in the compiled JAR files to a MOZ_FOO-specific @WrapForJNI method.  What that will mean is that your approach will fail if we have

{Nightly, MOZ_FOO=0}, {Beta, MOZ_FOO=1}

It will work if we have:

{Nightly, MOZ_FOO=1}, {Beta, MOZ_FOO=0}

The end game here can only be that the generated wrappers are produced by the build system, taking into account the relevant build flags, before they are consumed by the C++ compilation stage.  That's currently deep rocket science, because the Java code is compiled _after_ the C++ code (for completely opaque reasons about branding DTDs being built at various times, etc, etc).  I'm not saying that we have to solve this completely for this ticket, but I'm calling out that the partial solution shouldn't work in general and aligning people on the far future target.

jchen: am I wrong?
Flags: needinfo?(nchen)
(In reply to Nick Alexander :nalexander from comment #3)
> It will work if we have:
> 
> {Nightly, MOZ_FOO=1}, {Beta, MOZ_FOO=0}

You're correct that preprocessing is designed for this use case, which is what this bug is about.

> What that will mean is that your approach will fail if we have
> 
> {Nightly, MOZ_FOO=0}, {Beta, MOZ_FOO=1}

Right preprocessing wouldn't work here. Can you give an example where we do this? Skipping Nightly and shipping something directly in Beta sounds like a bad idea.
Flags: needinfo?(nchen)
(In reply to Jim Chen [:jchen] [:darchons] from comment #4)
> (In reply to Nick Alexander :nalexander from comment #3)
> > It will work if we have:
> > 
> > {Nightly, MOZ_FOO=1}, {Beta, MOZ_FOO=0}
> 
> You're correct that preprocessing is designed for this use case, which is
> what this bug is about.
> 
> > What that will mean is that your approach will fail if we have
> > 
> > {Nightly, MOZ_FOO=0}, {Beta, MOZ_FOO=1}
> 
> Right preprocessing wouldn't work here. Can you give an example where we do
> this? Skipping Nightly and shipping something directly in Beta sounds like a
> bad idea.

We do this right now: MOZ_INSTALL_TRACKING (Adjust SDK installation ping) is enabled in Release and Beta, disabled in Nightly.  Now that Nightly is in Google Play, that's probably not correct... but we don't want to be unable to have features on in "later" channels if they're "off" in earlier channels.
(In reply to Nick Alexander :nalexander from comment #5)
> but we don't want to be unable
> to have features on in "later" channels if they're "off" in earlier channels.

I guess with Adjust SDK, we trust that the external library will Just Work when we enable it on Beta. For our own code (i.e. code that may require JNI bindings), I still find it dangerous to enable something in Beta but disable it in Nightly.

I guess my point is the vast majority of scenarios will be Nightly-on / Beta-off, and preprocessing works well for that; it's okay if we don't support Nightly-off / Beta-on because it's a practice that should be discouraged, in addition to requiring more heavy lifting for it to work.
Comment on attachment 8875206 [details]
Bug 1368954 - [Part1] Use reflection to avoid build bustage when source code is only included in Nightly.

https://reviewboard.mozilla.org/r/146608/#review150622

For the case which source code is only included in nighlty but affecting generated JNI wrappers, this is an intermediate solution.
So that those java glue classes and generated data structure can be uplited without worring the missing of source code by feature flag.

John, Nick,
This is mainly related to the use of java languague. Could you review this for me ? or give me some suggestions.
Thanks.
Attachment #8875206 - Flags: review?(nalexander)
Attachment #8875206 - Flags: review?(jolin)
Comment on attachment 8875206 [details]
Bug 1368954 - [Part1] Use reflection to avoid build bustage when source code is only included in Nightly.

https://reviewboard.mozilla.org/r/146608/#review150654

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/BaseHlsPlayer.java:10
(Diff revision 1)
> +package org.mozilla.gecko.media;
> +
> +import android.util.Log;
> +import java.util.concurrent.ConcurrentLinkedQueue;
> +
> +public class BaseHlsPlayer {

Nit: since you're not going to instantiate `BaseHlsPlayer` anyway, How about making it an `interface` and keeping implementation stuff (instance variables, default method definitions, ...) in GeckoHlsPlayer?
Attachment #8875206 - Flags: review?(jolin) → review+
Comment on attachment 8875206 [details]
Bug 1368954 - [Part1] Use reflection to avoid build bustage when source code is only included in Nightly.

https://reviewboard.mozilla.org/r/146608/#review150674

Provide a static method to remove created player when it's not used.
Comment on attachment 8875206 [details]
Bug 1368954 - [Part1] Use reflection to avoid build bustage when source code is only included in Nightly.

https://reviewboard.mozilla.org/r/146606/#review150884

Make BaseHlsPlayer a simple interface.
Comment on attachment 8875206 [details]
Bug 1368954 - [Part1] Use reflection to avoid build bustage when source code is only included in Nightly.

https://reviewboard.mozilla.org/r/146608/#review150654

> Nit: since you're not going to instantiate `BaseHlsPlayer` anyway, How about making it an `interface` and keeping implementation stuff (instance variables, default method definitions, ...) in GeckoHlsPlayer?

Good idea ! Thanks
Assignee: nobody → kikuo
Comment on attachment 8875206 [details]
Bug 1368954 - [Part1] Use reflection to avoid build bustage when source code is only included in Nightly.

https://reviewboard.mozilla.org/r/146608/#review151386

The approach is good.  r- just so we can see the "after" of the non-trivial changes I've noted.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:46
(Diff revision 3)
>  import org.mozilla.gecko.AppConstants;
>  import org.mozilla.gecko.GeckoAppShell;
>  
>  import java.util.concurrent.ConcurrentLinkedQueue;
>  
> -public class GeckoHlsPlayer implements ExoPlayer.EventListener {
> +public class GeckoHlsPlayer implements BaseHlsPlayer, ExoPlayer.EventListener {

Annotate this with `@ReflectionTarget`.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:51
(Diff revision 3)
> -public class GeckoHlsPlayer implements ExoPlayer.EventListener {
> +public class GeckoHlsPlayer implements BaseHlsPlayer, ExoPlayer.EventListener {
>      private static final String LOGTAG = "GeckoHlsPlayer";
>      private static final DefaultBandwidthMeter BANDWIDTH_METER = new DefaultBandwidthMeter();
>      private static final int MAX_TIMELINE_ITEM_LINES = 3;
>      private static boolean DEBUG = false;
> +    private static int mId = 0;

This definitely deserves a comment explaining what the ID system is hoping to achieve.  Further, this absolutely should not be `mId`, since it's _not_ a member variable, it's static.

Are you confident that your `GeckoHlsPlayer` allocations will not race?  Should this be an `AtomicInteger`, or should access be synchronized by `GeckoHlsPlayer.class`?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:245
(Diff revision 3)
>          if (DEBUG) { Log.d(LOGTAG, " construct"); }
> +        mId++;
>      }
>  
> -    void addResourceWrapperCallbackListener(ResourceCallbacks callback) {
> +    @Override
> +    public int getId() {

Comment about your ID system here.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:568
(Diff revision 3)
>          }
>          return 0;
>      }
>  
> -    public Format getVideoTrackFormat(int index) {
> -        if (DEBUG) { Log.d(LOGTAG, "getVideoTrackFormat"); }
> +    @Override
> +    public GeckoVideoInfo getVideoInfo(int index) {

There seem to be changes that aren't just "shuffling where code lives" in here.  Can you explain why the method is changing name and growing functionality?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoPlayerFactory.java:36
(Diff revision 3)
> +        }
> +        return null;
> +    }
> +
> +    synchronized static void removePlayer(BaseHlsPlayer player) {
> +        int index = player != null ? sPlayerList.indexOf(player) : -1;

Do you need the null check?  Assuming you never _insert_ a `null` player, you can simplify this.  Consider using `@NonNull` for the argument, too.
Attachment #8875206 - Flags: review?(nalexander) → review-
Comment on attachment 8875206 [details]
Bug 1368954 - [Part1] Use reflection to avoid build bustage when source code is only included in Nightly.

https://reviewboard.mozilla.org/r/146608/#review151386

> This definitely deserves a comment explaining what the ID system is hoping to achieve.  Further, this absolutely should not be `mId`, since it's _not_ a member variable, it's static.
> 
> Are you confident that your `GeckoHlsPlayer` allocations will not race?  Should this be an `AtomicInteger`, or should access be synchronized by `GeckoHlsPlayer.class`?

Ah, the mId should be a non-static member variable. Thanks for pointing it out, I'll remove the static modifier.
The creation of GeckoHlsPlayer is encapsulated in GeckoPlayerFactory.getPlayer() and the access is synchronized.
I think these should be enough.

> There seem to be changes that aren't just "shuffling where code lives" in here.  Can you explain why the method is changing name and growing functionality?

In "before" code, Format is a class provided by ExoPlayer source code, then I convert Format to GeckoVideoInfo in GeckoHlsDemuxerWrapper which is the glue class that will affect generated JNI stuff.

Because I want to make the code compilable even if exoplayer source code and our implementation aren't there, I've changed the returned data type of the interface with {GeckoVideoInfo,GeckoAudioInfo}.
Then moved the part of data type convertion from GeckoHlsDemuxerWrapper to GeckoHLSPlayer to encapsulate the usage of everything of ExoPlayer in GekcoHlsPlayer.
Comment on attachment 8875206 [details]
Bug 1368954 - [Part1] Use reflection to avoid build bustage when source code is only included in Nightly.

https://reviewboard.mozilla.org/r/146608/#review151386

Thanks for pointing these things out ! Learnt :)
Comment on attachment 8875206 [details]
Bug 1368954 - [Part1] Use reflection to avoid build bustage when source code is only included in Nightly.

https://reviewboard.mozilla.org/r/146608/#review152612

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:56
(Diff revisions 3 - 4)
>      private static boolean DEBUG = false;
> -    private static int mId = 0;
>  
> +    /*
> +     *  Because we treat GeckoHlsPlayer as a source data provider.
> +     *  It will be created and initialized with an URL by HLSResource in

nit: "a URL".

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:252
(Diff revisions 3 - 4)
>          }
>      }
>  
>      public GeckoHlsPlayer() {
>          if (DEBUG) { Log.d(LOGTAG, " construct"); }
>          mId++;

You've just made this a member variable, so this doesn't do what you want: every `GeckoHlsPlayer` instance will have `mId = 1`.

You need a global static identifer _and_ a per-instance identifier for this scheme to work.

I'm worried that your testing is not sufficient, either automated or manual testing.  Clearly you can't be trying, say, two videos on the same page at the same time, since you'd witness the `mId` mismatch if this was true.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsResourceWrapper.java:58
(Diff revision 4)
>      public static GeckoHlsResourceWrapper create(String url,
> -                                                 GeckoHlsPlayer.ResourceCallbacks callback) {
> +                                                 HlsResourceCallbacks callback) {
>          return new GeckoHlsResourceWrapper(url, callback);
>      }
>  
>      @WrapForJNI(calledFrom = "gecko")

I expect native code changes to accommodate your new interface.

Is this not actually invoked yet?  Future work?
Attachment #8875206 - Flags: review?(nalexander) → review-
(In reply to Nick Alexander :nalexander from comment #19)
> Comment on attachment 8875206 [details]
> Bug 1368954 - Use reflection to avoid build bustage when source code is only
> included in Nightly.
> 
> https://reviewboard.mozilla.org/r/146608/#review152612
> 
> :::
> mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/
> GeckoHlsPlayer.java:56
> (Diff revisions 3 - 4)
> >      private static boolean DEBUG = false;
> > -    private static int mId = 0;
> >  
> > +    /*
> > +     *  Because we treat GeckoHlsPlayer as a source data provider.
> > +     *  It will be created and initialized with an URL by HLSResource in
> 
> nit: "a URL".
> 
> :::
> mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/
> GeckoHlsPlayer.java:252
> (Diff revisions 3 - 4)
> >          }
> >      }
> >  
> >      public GeckoHlsPlayer() {
> >          if (DEBUG) { Log.d(LOGTAG, " construct"); }
> >          mId++;
> 
> You've just made this a member variable, so this doesn't do what you want:
> every `GeckoHlsPlayer` instance will have `mId = 1`.

Oh my gosh ...  what I've replied/done was totally wrong(stupid) : (
Sorry about that, I replied it while I was participating a Python conference and had a serious headache then.
That variable 'mId' should be definitely a static 'sId' and the access to creation is through 'synchronized'.

> 
> You need a global static identifer _and_ a per-instance identifier for this
> scheme to work.
> 
> I'm worried that your testing is not sufficient, either automated or manual
> testing.  Clearly you can't be trying, say, two videos on the same page at
> the same time, since you'd witness the `mId` mismatch if this was true.
> 

We have not landed any test cases for this yet, but we will. 
Bug 1372465 is filed to address the issue here.

> :::
> mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/
> GeckoHlsResourceWrapper.java:58
> (Diff revision 4)
> >      public static GeckoHlsResourceWrapper create(String url,
> > -                                                 GeckoHlsPlayer.ResourceCallbacks callback) {
> > +                                                 HlsResourceCallbacks callback) {
> >          return new GeckoHlsResourceWrapper(url, callback);
> >      }
> >  
> >      @WrapForJNI(calledFrom = "gecko")
> 
> I expect native code changes to accommodate your new interface.
> 
> Is this not actually invoked yet?  Future work?

Bug 1350246 is the native part of this feature. I'd wait until it landed (should be today) and rebase then make a change to it.
Attachment #8877502 - Flags: review?(jacheng)
Attachment #8875206 - Flags: review?(nalexander)
Comment on attachment 8875206 [details]
Bug 1368954 - [Part1] Use reflection to avoid build bustage when source code is only included in Nightly.

https://reviewboard.mozilla.org/r/146608/#review152612

> You've just made this a member variable, so this doesn't do what you want: every `GeckoHlsPlayer` instance will have `mId = 1`.
> 
> You need a global static identifer _and_ a per-instance identifier for this scheme to work.
> 
> I'm worried that your testing is not sufficient, either automated or manual testing.  Clearly you can't be trying, say, two videos on the same page at the same time, since you'd witness the `mId` mismatch if this was true.

Yes, there should be a member variable and a static variable to make the scheme work. I've addressed it. Sorry for my casualness about last push-review.  
Regarding the test case, I'm thinking to play 2 videos with different resolution, I'll work on that in Bug 1372465.

> I expect native code changes to accommodate your new interface.
> 
> Is this not actually invoked yet?  Future work?

Since Bug 1350246 was landed, I corrected affected code in Part2.
Comment on attachment 8877502 [details]
Bug 1368954 - [Part2] Modify HLSDemuxer native code to accommondate wrapper interface change.

https://reviewboard.mozilla.org/r/148944/#review153410

::: dom/media/hls/HLSDecoder.cpp:18
(Diff revision 1)
>  #include "MediaContainerType.h"
>  #include "MediaDecoderStateMachine.h"
>  #include "MediaFormatReader.h"
>  #include "MediaPrefs.h"
>  
> +using namespace mozilla::java;

Is it necessary?
Attachment #8877502 - Flags: review?(jacheng) → review+
Comment on attachment 8877502 [details]
Bug 1368954 - [Part2] Modify HLSDemuxer native code to accommondate wrapper interface change.

https://reviewboard.mozilla.org/r/148944/#review153414

awesome
Comment on attachment 8875206 [details]
Bug 1368954 - [Part1] Use reflection to avoid build bustage when source code is only included in Nightly.

https://reviewboard.mozilla.org/r/146608/#review153512

I mostly just re-read the player ID allocation.  I think it's still not correct, but I don't think you need my re-review to get it correct.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHLSDemuxerWrapper.java:34
(Diff revision 5)
>          public int value() {
>              return mType;
>          }
>      }
>  
> -    private GeckoHlsPlayer mPlayer = null;
> +    private BaseHlsPlayer mPlayer = null;

In the interdiff, you made a whole bunch of s/Hls/HLS/ ... and now you are leaving this as `BaseHlsPlayer`.  I see you have both `Hls` and `HLS` in this patch.

Figure out what names you want, and then make a Post: patch that does _nothing_ except renaming.  Each version of this patch series makes many unrelated changes that don't seem strictly necessary.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:262
(Diff revision 5)
>          }
>      }
>  
> -    GeckoHlsPlayer() {
> +    public GeckoHlsPlayer() {
>          if (DEBUG) { Log.d(LOGTAG, " construct"); }
> +        mPlayerId = ++sPlayerId;

++ is not atomic in Java: see, for example, https://stackoverflow.com/a/25171089.  I don't see how you're guarding `sPlayerId`.  Can you explain to me why this isn't a race condition?  Is there a reason this isn't an `AtomicInteger`?  (I feel like you did, and I've forgotten.)

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoPlayerFactory.java:22
(Diff revision 5)
> +            final Class<?> cls = Class.forName("org.mozilla.gecko.media.GeckoHlsPlayer");
> +            BaseHlsPlayer player = (BaseHlsPlayer) cls.newInstance();
> +            sPlayerList.add(player);
> +            return player;
> +        } catch (Exception e) {
> +            Log.e("GeckoPlayerFactory", "Found no player!", e);

This is misleading, given that you have `getPlayer` that _searches_ for players.  Maybe "Could not find GeckoHlsPlayer class"?  Maybe the exception details will be enough.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoPlayerFactory.java:27
(Diff revision 5)
> +            Log.e("GeckoPlayerFactory", "Found no player!", e);
> +        }
> +        return null;
> +    }
> +
> +    synchronized static BaseHlsPlayer getPlayer(int id) {

The consumer I see that uses this will immediately NPE if the player isn't found.  Can we at least try to help people out, by logging "No player found with id XXX"?  And including the id in the "creating player" and "destroying player" messages?

You don't have to do this, but I feel like you're making your future life as you try to bring this feature to stability harder.
Attachment #8875206 - Flags: review+
Comment on attachment 8875206 [details]
Bug 1368954 - [Part1] Use reflection to avoid build bustage when source code is only included in Nightly.

https://reviewboard.mozilla.org/r/146608/#review153512

> In the interdiff, you made a whole bunch of s/Hls/HLS/ ... and now you are leaving this as `BaseHlsPlayer`.  I see you have both `Hls` and `HLS` in this patch.
> 
> Figure out what names you want, and then make a Post: patch that does _nothing_ except renaming.  Each version of this patch series makes many unrelated changes that don't seem strictly necessary.

Bug 1368907 was landed when I was working on this bug.  James and I had a discussion, we'd also like to respect the naming convention from ExoPlayer, so we use _Hls_ for those components which we inject them as customized components.
Then for those components i.e. gecko java wrappers and gecko native components, we use _HLS_.

That was my intension.  Do you think if it's ok to leave it as it is ?

> ++ is not atomic in Java: see, for example, https://stackoverflow.com/a/25171089.  I don't see how you're guarding `sPlayerId`.  Can you explain to me why this isn't a race condition?  Is there a reason this isn't an `AtomicInteger`?  (I feel like you did, and I've forgotten.)

GeckoHlsPlayer is only created in GeckoPlayerFactory.getPlayer() and the access to all GeckoPlayerFactory's APIs is synchronized.
I could make sPlayerId an AtomicInterger to avoid the confusion.
Thanks for the note, learnt : )

> The consumer I see that uses this will immediately NPE if the player isn't found.  Can we at least try to help people out, by logging "No player found with id XXX"?  And including the id in the "creating player" and "destroying player" messages?
> 
> You don't have to do this, but I feel like you're making your future life as you try to bring this feature to stability harder.

Hmm indeed, thanks for the suggestion, I'll address it :)
Comment on attachment 8875206 [details]
Bug 1368954 - [Part1] Use reflection to avoid build bustage when source code is only included in Nightly.

https://reviewboard.mozilla.org/r/146608/#review153512

> Bug 1368907 was landed when I was working on this bug.  James and I had a discussion, we'd also like to respect the naming convention from ExoPlayer, so we use _Hls_ for those components which we inject them as customized components.
> Then for those components i.e. gecko java wrappers and gecko native components, we use _HLS_.
> 
> That was my intension.  Do you think if it's ok to leave it as it is ?

OK.  Your team has a vision here and has come to a decision.  Roll on!

> GeckoHlsPlayer is only created in GeckoPlayerFactory.getPlayer() and the access to all GeckoPlayerFactory's APIs is synchronized.
> I could make sPlayerId an AtomicInterger to avoid the confusion.
> Thanks for the note, learnt : )

Your choice.  `AtomicInteger` is transparent; otherwise you need to comment on the concurrency conditions, and ensure they don't change as the code evolves.
Comment on attachment 8875206 [details]
Bug 1368954 - [Part1] Use reflection to avoid build bustage when source code is only included in Nightly.

https://reviewboard.mozilla.org/r/146606/#review153752

Thank you so much for the review :)
Android lint errors are shown in the try result, but it's not related to this Bug, tracked in Bug 1373143.
Pushed by kikuo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/368732039445
[Part1] Use reflection to avoid build bustage when source code is only included in Nightly. r=jolin,nalexander
https://hg.mozilla.org/integration/autoland/rev/95346df93cfd
[Part2] Modify HLSDemuxer native code to accommondate wrapper interface change. r=JamesCheng
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: