Closed Bug 1264596 Opened 5 years ago Closed 4 years ago

GeckoApp: Remove mCameraView and related code

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox52 fixed)

RESOLVED FIXED
Firefox 52
Tracking Status
firefox52 --- fixed

People

(Reporter: sebastian, Assigned: shatur)

References

Details

Attachments

(1 file, 1 obsolete file)

After bug 1261040 lands mCameraView will not be set any more (was only used on devices with Android < 4). Let's remove this and related code (and layouts).
Please assign this to me. Thanks.
Assignee: nobody → jorick.caberio
Status: NEW → ASSIGNED
Assignee: jorick.caberio → nobody
Mentor: s.kaspari
Status: ASSIGNED → NEW
Attached patch Raw.patch (obsolete) — Splinter Review
Hi Sebastian,
   I have some questions, like do I have to remove all related code to mCamera [like getCameraView, enableCameraview, etc, method from geckAppshell.java and other files] or just this which I have removed in this patch.

Regards.
Attachment #8799477 - Flags: review?(s.kaspari)
Comment on attachment 8799477 [details] [diff] [review]
Raw.patch

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

> I have some questions, like do I have to remove all related code to
> mCamera [like getCameraView, enableCameraview, etc, method from
> geckAppshell.java and other files] or just this which I have removed in this
> patch.

Yeah, after the changes getCameraView() should always return null, so you can remove even more code.

Please verify your changes with this test page (Video sharing should still work):
http://mozilla.github.io/webrtc-landing/gum_test.html

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
@@ -1862,5 @@
> -                    }
> -                }
> -            }
> -        };
> -        mCameraOrientationEventListener.enable();

Are you sure that this listener is not used anymore? It looks like there can be listeners in GeckoAppShell that still want to know about orientation changes (or did you verify that those are only used for CameraView related code?).
Attachment #8799477 - Flags: review?(s.kaspari) → feedback+
Assignee: nobody → tushar.saini1285
Status: NEW → ASSIGNED
Attachment #8799477 - Attachment is obsolete: true
Comment on attachment 8801751 [details]
Bug 1264596 - GeckoApp: Remove mCameraView and related code.

https://reviewboard.mozilla.org/r/86406/#review85550

::: media/webrtc/trunk/webrtc/modules/video_render/android/java/src/org/webrtc/videoengine/ViERenderer.java:42
(Diff revision 1)
>      // Creates a SurfaceView to be used by Android Camera
>      // service to display a local preview.
>      // This needs to be used on Android prior to version 2.1
>      // in order to run the camera.

We should update/remove this part of the comment as we do not create a SurfaceView anymore and this isn't for Android < 2.1 anymore.

::: media/webrtc/trunk/webrtc/modules/video_render/android/java/src/org/webrtc/videoengine/ViERenderer.java
(Diff revision 1)
>      // ViECapture::StartCapture
>      public static void CreateLocalRenderer() {
> -        View cameraView = GeckoAppShell.getGeckoInterface().getCameraView();
> -        if (cameraView != null && (cameraView instanceof SurfaceView)) {
> -            SurfaceView localRender = (SurfaceView)cameraView;
> -            g_localRenderer = localRender.getHolder();

After that g_localRenderer won't be set anymore an dyou can remove more code in DestroyLocalRenderer(). Note that we should still start/stop the orientation sensor (via enable/disableCameraView). Maybe it makes sense to rename enable/disableCameraView because after the refactoring it will only start stop the orientation listener.

Additionally the CreateRenderer() and GetLocalRenderer() methods seem to be unused here.

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:336
(Diff revision 1)
>      public View getCameraView() {
> -        return mCameraView;
> +        return null;
>      }

Now that this method always returns null you should be able to remove it and all code that expected non null return values.
Attachment #8801751 - Flags: review?(s.kaspari)
Comment on attachment 8801751 [details]
Bug 1264596 - GeckoApp: Remove mCameraView and related code.

https://reviewboard.mozilla.org/r/86406/#review85872

This looks good to me. Thank your for this patch. I'll push it to try now.

Because this touches webrtc I'll flag gcp as a second reviewer.
Attachment #8801751 - Flags: review?(s.kaspari) → review+
Attachment #8801751 - Flags: review?(gpascutto)
Comment on attachment 8801751 [details]
Bug 1264596 - GeckoApp: Remove mCameraView and related code.

https://reviewboard.mozilla.org/r/86406/#review86224

There's a few if SDK_INT>8 in this code too. If you want to, removing those is a nice follow up patch or bug.

::: media/webrtc/trunk/webrtc/modules/video_capture/android/java/src/org/webrtc/videoengine/VideoCaptureAndroid.java:203
(Diff revision 2)
> -        if (localPreview.getSurface() != null &&
> -            localPreview.getSurface().isValid()) {
> -          camera.setPreviewDisplay(localPreview);
> -        }
> -      } else {
> -        if(android.os.Build.VERSION.SDK_INT>10) {
> +      if(android.os.Build.VERSION.SDK_INT>10) {

This if can go, as we only support devices where it's true anyway.

::: media/webrtc/trunk/webrtc/modules/video_capture/android/java/src/org/webrtc/videoengine/VideoCaptureAndroid.java:384
(Diff revision 2)
>        camera.stopPreview();
> -      if (localPreview != null) {
> -        localPreview.removeCallback(this);
> -        camera.setPreviewDisplay(null);
> -      } else {
> -        if(android.os.Build.VERSION.SDK_INT>10) {
> +      if(android.os.Build.VERSION.SDK_INT>10) {

Same remark.

::: media/webrtc/trunk/webrtc/modules/video_render/android/java/src/org/webrtc/videoengine/ViERenderer.java:43
(Diff revision 2)
>              @Override
>              public void run() {
>                  try {
> -                    GeckoAppShell.getGeckoInterface().enableCameraView();
> +                    GeckoAppShell.getGeckoInterface().enableOrientationListener();
>                  } catch (Exception e) {
> -                    Log.e(TAG, "CreateLocalRenderer enableCameraView exception: "
> +                    Log.e(TAG, "Create enableOrientationListener exception: "

"Create enable" sounds weird (and it's wrong, because enableOrientationListener is not what is created, it's an OrientationListener). Remove the "create" part of the message.

::: media/webrtc/trunk/webrtc/modules/video_render/android/java/src/org/webrtc/videoengine/ViERenderer.java:58
(Diff revision 2)
> -                public void run() {
> +            public void run() {
> -                    try {
> +                try {
> -                        GeckoAppShell.getGeckoInterface().disableCameraView();
> +                    GeckoAppShell.getGeckoInterface().disableOrientationListener();
> -                    } catch (Exception e) {
> +                } catch (Exception e) {
> -                        Log.e(TAG,
> +                    Log.e(TAG,
> -                              "DestroyLocalRenderer disableCameraView exception: " +
> +                          "Destroy disableOrientationListener exception: " +

Same remark.
Attachment #8801751 - Flags: review?(gpascutto) → review+
Pushed by s.kaspari@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7cd638c8fb12
GeckoApp: Remove mCameraView and related code. r=gcp,sebastian
https://hg.mozilla.org/mozilla-central/rev/7cd638c8fb12
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.