1297969 - Extract class RemoteManager from CodecProxy.java to RemoteManager.java for further usage

Status: RESOLVED FIXED

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

Description

[James Cheng[:JamesCheng]]

For extensible, we would like to make this change for further MediaDrmBridge out of process usage (Bug 1292076).

Comment 1

[James Cheng[:JamesCheng]]

Attachment: Bug 1297969 - Extract class RemoteManager from CodecProxy.java to RemoteManager.java for further usage.

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

Comment 2

[John Lin [:jhlin][:jolin]]

Comment on attachment 8784711 [details]
Bug 1297969 - Extract class RemoteManager from CodecProxy.java to RemoteManager.java for further usage.

https://reviewboard.mozilla.org/r/74046/#review71950

Nice work. Overall looks good to me.

::: mobile/android/base/java/org/mozilla/gecko/media/CodecProxy.java:94
(Diff revision 1)
> -        }
> -    }
> -
>      @WrapForJNI
>      public static CodecProxy create(MediaFormat format, Surface surface, Callbacks callbacks) {
> -        if (!sRemoteManager.init()) {
> +        if (!RemoteManager.getRemoteManager().init()) {

Suggest to name the getter `RemoteManager.getInstance()` and call `RemoteManager.init()` in it.

::: mobile/android/base/java/org/mozilla/gecko/media/CodecProxy.java:110
(Diff revision 1)
>          mFormat = new FormatParam(format);
>          mOutputSurface = surface;
>          mCallbacks = new CallbacksForwarder(callbacks);
>      }
>  
> -    private boolean init(ICodec remote) {
> +    public boolean init(ICodec remote) {

Nit: default (package) access should be enough.

::: mobile/android/base/java/org/mozilla/gecko/media/CodecProxy.java:124
(Diff revision 1)
>  
>          mRemote = remote;
>          return true;
>      }
>  
> -    private boolean deinit() {
> +    public boolean deinit() {

Ditto.

Comment 3

[James Cheng[:JamesCheng]]

Comment on attachment 8784711 [details]
Bug 1297969 - Extract class RemoteManager from CodecProxy.java to RemoteManager.java for further usage.

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

Comment 4

[John Lin [:jhlin][:jolin]]

Comment on attachment 8784711 [details]
Bug 1297969 - Extract class RemoteManager from CodecProxy.java to RemoteManager.java for further usage.

https://reviewboard.mozilla.org/r/74046/#review71964

On second thought, let getInstance() return null doesn't sound like a good idea.
How about returning a RemoteManager that createCodec() always returns null and releaseCodec() does nothing at all when it failed to init()?
This would make CodecProxy code simpler.

Comment 5

[John Lin [:jhlin][:jolin]]

Comment on attachment 8784711 [details]
Bug 1297969 - Extract class RemoteManager from CodecProxy.java to RemoteManager.java for further usage.

https://reviewboard.mozilla.org/r/74046/#review71966

Comment 6

[John Lin [:jhlin][:jolin]]

Comment on attachment 8784711 [details]
Bug 1297969 - Extract class RemoteManager from CodecProxy.java to RemoteManager.java for further usage.

https://reviewboard.mozilla.org/r/74046/#review71982

::: mobile/android/base/java/org/mozilla/gecko/media/RemoteManager.java:31
(Diff revisions 1 - 2)
>      private static final String LOGTAG = "GeckoRemoteManager";
>      private static final boolean DEBUG = false;
>      private static RemoteManager sRemoteManager = null;
>  
> -    public static RemoteManager getRemoteManager() {
> +    public static RemoteManager getInstance() {
>          if (sRemoteManager == null){

Why check nullness twice here and the line after next?

Comment 7

[James Cheng[:JamesCheng]]

(In reply to John Lin [:jolin][:jhlin] from comment #6)
> Comment on attachment 8784711 [details]
> Bug 1297969 - Extract class RemoteManager from CodecProxy.java to
> RemoteManager.java for further usage.
> 
> https://reviewboard.mozilla.org/r/74046/#review71982
> 
> ::: mobile/android/base/java/org/mozilla/gecko/media/RemoteManager.java:31
> (Diff revisions 1 - 2)
> >      private static final String LOGTAG = "GeckoRemoteManager";
> >      private static final boolean DEBUG = false;
> >      private static RemoteManager sRemoteManager = null;
> >  
> > -    public static RemoteManager getRemoteManager() {
> > +    public static RemoteManager getInstance() {
> >          if (sRemoteManager == null){
> 
> Why check nullness twice here and the line after next?

It's just an approach to avoid race condition called double-check locking with lazy initialization.

Could I leave this code or have better suggestion?

Thanks

Comment 8

[John Lin [:jhlin][:jolin]]

(In reply to James Cheng[:JamesCheng] from comment #7)
> (In reply to John Lin [:jolin][:jhlin] from comment #6)
> > Comment on attachment 8784711 [details]
> > Bug 1297969 - Extract class RemoteManager from CodecProxy.java to
> > RemoteManager.java for further usage.
> > 
> > https://reviewboard.mozilla.org/r/74046/#review71982
> > 
> > ::: mobile/android/base/java/org/mozilla/gecko/media/RemoteManager.java:31
> > (Diff revisions 1 - 2)
> > >      private static final String LOGTAG = "GeckoRemoteManager";
> > >      private static final boolean DEBUG = false;
> > >      private static RemoteManager sRemoteManager = null;
> > >  
> > > -    public static RemoteManager getRemoteManager() {
> > > +    public static RemoteManager getInstance() {
> > >          if (sRemoteManager == null){
> > 
> > Why check nullness twice here and the line after next?
> 
> It's just an approach to avoid race condition called double-check locking
> with lazy initialization.
> 
> Could I leave this code or have better suggestion?
> 
> Thanks

 Up to you. Double checked locking makes the implementation look a little complicated and in this case I prefer simpler code (synchronized statements) over optimization benefits.

Comment 9

[James Cheng[:JamesCheng]]

Comment on attachment 8784711 [details]
Bug 1297969 - Extract class RemoteManager from CodecProxy.java to RemoteManager.java for further usage.

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

Comment 10

[John Lin [:jhlin][:jolin]]

Comment on attachment 8784711 [details]
Bug 1297969 - Extract class RemoteManager from CodecProxy.java to RemoteManager.java for further usage.

https://reviewboard.mozilla.org/r/74046/#review72318

::: mobile/android/base/java/org/mozilla/gecko/media/RemoteManager.java:35
(Diff revisions 2 - 3)
> -                      return null;
> -                    }
> -                }
> -            }
> +        }
> +        // Need to init due to release called or first time using.
> +        if (sRemoteManager.mRemote == null) {

I guess this check is not necessary because `new` never returns `null`. :)

Comment 11

[John Lin [:jhlin][:jolin]]

Comment on attachment 8784711 [details]
Bug 1297969 - Extract class RemoteManager from CodecProxy.java to RemoteManager.java for further usage.

https://reviewboard.mozilla.org/r/74046/#review72322

Thanks!

Comment 12

[James Cheng[:JamesCheng]]

Comment on attachment 8784711 [details]
Bug 1297969 - Extract class RemoteManager from CodecProxy.java to RemoteManager.java for further usage.

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

Comment 13

[James Cheng[:JamesCheng]]

Thank you for the feedback and reviewing.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4542cf942dab
Extract class RemoteManager from CodecProxy.java to RemoteManager.java for further usage. r=jolin

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out for somehow breaking the Android checkstyle job.
https://treeherder.mozilla.org/logviewer.html#?job_id=2702943&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/3aa804757b5b

Comment 16

[Ryan VanderMeulen [:RyanVM]]

19:10:52 INFO - :app:checkstyle[ant:checkstyle] /home/worker/workspace/build/src/mobile/android/base/java/org/mozilla/gecko/media/RemoteManager.java:31:36: '{' is not preceded with whitespace.

Comment 17

[James Cheng[:JamesCheng]]

Ensure it passed the checkstyle check,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b3633031879

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/54752c3b692e
Extract class RemoteManager from CodecProxy.java to RemoteManager.java for further usage. r=jolin

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/54752c3b692e