Closed Bug 1297969 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: JamesCheng, Assigned: JamesCheng)

References

Details

Attachments

(1 file)

For extensible, we would like to make this change for further MediaDrmBridge out of process usage (Bug 1292076).
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.
Attachment #8784711 - Flags: review?(jolin) → review+
Priority: -- → P3
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 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
Attachment #8784711 - Flags: review+ → review?(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?
Attachment #8784711 - Flags: review?(jolin) → review-
(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
Flags: needinfo?(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.
Flags: needinfo?(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`. :)
Attachment #8784711 - Flags: review?(jolin) → review-
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!
Attachment #8784711 - Flags: review- → review+
Thank you for the feedback and reviewing.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4542cf942dab
Extract class RemoteManager from CodecProxy.java to RemoteManager.java for further usage. r=jolin
Keywords: checkin-needed
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.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/54752c3b692e
Extract class RemoteManager from CodecProxy.java to RemoteManager.java for further usage. r=jolin
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/54752c3b692e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Blocks: 1299386
You need to log in before you can comment on or make changes to this bug.