1186826 - Implement an abstraction object for NativeWindow to hide hardware dependent operations

Status: RESOLVED WONTFIX

(Core Graveyard :: Widget: Gonk, defect)

Description

[Vincent Liu[:vliu]]

Currently Gecko contains different android version of NativeWindow. To keep maintaining these difference in the same branch, apps like Camera/Video needs to use compiler flag to fit for all situations for using NativeWindow.

In this bug, I want to implement an abstraction object to deal with these kind of hardware dependent interfaces. With the methods in this abstraction object exposing in Gecko, apps like Camera/Video don't need to consider the difference caused by different android version.

Comment 1

[Vincent Liu[:vliu]]

Attachment: WIP1-v1.patch

Hi CJ, John,

Here to attach a WIP_v1 to come out my idea. This WIP only covers the use case for camera and it works for KK an L. I want to ask for more comments to know what is the better to do it. 

Currently the name of this abstraction object seems not the perfect one because I have no idea what is the better name to naming. Maybe you can also have comment for it. Really thanks.

Comment 2

[u459114]

Comment on attachment 8637778 [details] [diff] [review]
WIP1-v1.patch

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

::: dom/camera/GonkCameraHwMgr.cpp
@@ +204,4 @@
>    // Disable shutter sound in android CameraService because gaia camera app will play it
>    mCamera->sendCommand(CAMERA_CMD_ENABLE_SHUTTER_SOUND, 0, 0);
>  
> +  mNativeWindowHal = new GeckoNativeWindowHal(GonkCameraHardware::MIN_UNDEQUEUED_BUFFERS);

If you want to isolate GeckoNativeWindowHal in a separate so file, and my binary replace that so without recompile geck, you may not "new" that obejct directly. Please considerate object factory pattern.

::: widget/gonk/hal/nativewindowhal/NativewindowHal.h
@@ +45,5 @@
> +     sp<GonkNativeWindow>          mNativeWindow;
> +#if ANDROID_VERSION >= 21
> +     sp<IGraphicBufferProducer> mProducer;
> +     sp<IGonkGraphicBufferConsumer> mConsumer;
> +#endif         

Use interface concept here(or pimp). There should be no member data for abstraction layer class/interface.

interface GeckoNativeWindowHal {
  // declare virtual function only.
}

GeckiNativeWindowHal *CreateNativeWindow();

Comment 3

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

Comment on attachment 8637778 [details] [diff] [review]
WIP1-v1.patch

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

I guess you can name it INativeWindow f you take CJ's idea of interface. :)

::: dom/camera/GonkCameraHwMgr.cpp
@@ +72,4 @@
>    if (mClosing) {
>      return;
>    }
> +  RefPtr<TextureClient> buffer = mNativeWindowHal->getNativeWindow()->getCurrentBuffer();

How about NativeWindowHal::getCurrentBuffer()?

@@ +206,5 @@
>  
> +  mNativeWindowHal = new GeckoNativeWindowHal(GonkCameraHardware::MIN_UNDEQUEUED_BUFFERS);
> +  mNativeWindowHal->setBufferQueueSynchronousMode(false);
> +
> +#if ANDROID_VERSION >= 19

It's unfortunate camera also has different API (setPreviewXXX()). IMHO this should be fixed too.

@@ +216,2 @@
>  #endif
> +  mNativeWindowHal->getNativeWindow()->setNewFrameCallback(this);

NativeWindowHal::setNewFrameCallback()

@@ +281,3 @@
>    }
>    mCamera.clear();
>  #ifdef MOZ_WIDGET_GONK

Is this necessary? GonkCameraHardware is only available in Gonk, after all.

@@ +288,1 @@
>  

I would prefer NativeWindowHal::abandon()

@@ +296,4 @@
>    DOM_CAMERA_LOGT("%s:%d : this=%p\n", __func__, __LINE__, (void*)this);
>    mCamera.clear();
>  #ifdef MOZ_WIDGET_GONK
> +  mNativeWindowHal->getNativeWindow().clear();

Ditto.

::: widget/gonk/hal/nativewindowhal/NativewindowHal.h
@@ +32,5 @@
> +public:
> +     GeckoNativeWindowHal(int aBufferCount);
> +     ~GeckoNativeWindowHal();
> +
> +     sp<GonkNativeWindow>          getNativeWindow()     { return mNativeWindow; };

IHMO exposing this defeats the whole purpose of hiding impl.

Comment 4

[Vincent Liu[:vliu]]

Attachment: WIP-v2.patch

Based on the addressed comment for WIP_v1, WIP_v2 was attached. In this patch, a static point gotten by GonkBufferQueueHalBase::CreateGonkBufferQueueHal() to manipulate its method. Could you please also give me feedback for any suggestion? Thanks.

Comment 5

[u459114]

Comment on attachment 8642865 [details] [diff] [review]
WIP-v2.patch

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

::: widget/gonk/hal/GonkBufferQueueHal/GonkBufferQueueHal.h
@@ +39,5 @@
> +     virtual sp<IGraphicBufferProducer>      getBufferProducer() = 0;
> +#else
> +     virtual sp<GonkBufferQueue>             getBufferProducer() = 0;
> +#endif
> +

We shouldn't have compile flag in an interface.

@@ +47,5 @@
> +     virtual void                            Clear() = 0;
> +     virtual void                            Abandon() = 0;         
> +};
> +
> +class GonkBufferQueueHal : public GonkBufferQueueHalBase

Don't put interface(GonkBufferQueueHalBase) and concrete implement in the same header(GonkBufferQueueHal).
Ideally, an exported header consists of only interfaces and creator function.

Comment 6

[Vincent Liu[:vliu]]

Attachment: WIP-v3.patch

Based on the addressed comment, the WIP_v3 was attached.

> ::: widget/gonk/hal/GonkBufferQueueHal/GonkBufferQueueHal.h
> @@ +39,5 @@
> > +     virtual sp<IGraphicBufferProducer>      getBufferProducer() = 0;
> > +#else
> > +     virtual sp<GonkBufferQueue>             getBufferProducer() = 0;
> > +#endif
> > +

These two methods can be merged into one since they all inherited from BnGraphicBufferProducer.

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Vincent,

do you think it would be possible to move the window code into a separate system daemon? I recently opened bug 1187799 to run Android drivers in their own address space? Graphics (gralloc and camera) was on my list, but I did not know how much work it is.

The idea of these daemon is that there's a daemon for each system (ICS, JB, KK, L, ...). all daemons expose the same same IPC interfaces to Gecko. Gecko would use whatever daemon is installed on the system. All dependencies on Android or hardware are encapsulated on the daemon. 

We already have separate system daemons for NFC and Bluetooth, a framework for writing new daemons, and experience how to do it. Graphics would be a welcome addition.

Comment 8

[Vincent Liu[:vliu]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #7)
> Hi Vincent,
> 
> do you think it would be possible to move the window code into a separate
> system daemon? I recently opened bug 1187799 to run Android drivers in their
> own address space? Graphics (gralloc and camera) was on my list, but I did
> not know how much work it is.
> 

Actually I still collect the list for gfx relative part for my next move. If you have any cool idea specifically need to be moved out of Gecko, you can file bug and CC me.

> The idea of these daemon is that there's a daemon for each system (ICS, JB,
> KK, L, ...). all daemons expose the same same IPC interfaces to Gecko. Gecko
> would use whatever daemon is installed on the system. All dependencies on
> Android or hardware are encapsulated on the daemon. 
> 
> We already have separate system daemons for NFC and Bluetooth, a framework
> for writing new daemons, and experience how to do it. Graphics would be a
> welcome addition.

I am not sure new daemon architecture also fit for Graphics/Camera when we talk about performance. Currently I prefer building as .so file and manipulating methods by static pointer. Please see this bug for my idea.

Comment 9

[u459114]

Comment on attachment 8643578 [details] [diff] [review]
WIP-v3.patch

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

::: dom/camera/GonkCameraHwMgr.cpp
@@ +210,5 @@
> +    mBufferQueueHal->InitBufferQueue(GonkCameraHardware::MIN_UNDEQUEUED_BUFFERS);
> +    mBufferQueueHal->setBufferQueueSynchronousMode(false);
> +  }
> +
> +#if ANDROID_VERSION >= 19

Why we still need ANDROID_VERSION here?

::: widget/gonk/hal/GonkBufferQueueHal/GonkBufferQueueHal.h
@@ +39,5 @@
> +    virtual void                            setNewFrameCallback(GonkNativeWindowNewFrameCallback* callback) override;
> +    virtual already_AddRefed<mozilla::layers::TextureClient> getCurrentBuffer() override;
> +    virtual void                            Clear() override;
> +    virtual void                            Abandon() override;
> +protected:

private

::: widget/gonk/hal/GonkBufferQueueHal/GonkBufferQueueHalBase.h
@@ +18,5 @@
> +#ifndef GONK_BUFFERQUEUE_HAL_BASE
> +#define GONK_BUFFERQUEUE_HAL_BASE
> +
> +#include "GonkNativeWindow.h"
> +#if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 21

Ideally, client side which include this header should not be aware of android version.

@@ +23,5 @@
> +#include "GonkBufferQueueProducer.h"
> +#endif
> +
> +namespace android {
> +class GonkBufferQueueHalBase : public RefBase

Are you sure you want inherit from RefBase?
Rename GonkBufferQueueHalBase to GonkBufferQueueHal
Rename GonkBufferQueueHal to GonkBufferQueueHalImp

@@ +28,5 @@
> +{
> +public:
> +    explicit GonkBufferQueueHalBase() = default;
> +    virtual  ~GonkBufferQueueHalBase() {}
> +

Why we need constructor and destructor here?

@@ +29,5 @@
> +public:
> +    explicit GonkBufferQueueHalBase() = default;
> +    virtual  ~GonkBufferQueueHalBase() {}
> +
> +    static  GonkBufferQueueHalBase*         CreateGonkBufferQueueHal();

I would suggest pull out this static function.

@@ +34,5 @@
> +    virtual void                            InitBufferQueue(int aBufferCount) = 0;
> +    virtual sp<GonkNativeWindow>            getNativeWindow() = 0;
> +    virtual sp<IGraphicBufferProducer>      getBufferProducer() = 0;
> +    virtual void                            setBufferQueueSynchronousMode(bool aEnabled) = 0;
> +    virtual void                            setNewFrameCallback(GonkNativeWindowNewFrameCallback* callback) = 0;

May I know where is the definition of GonkNativeWindowNewFrameCallback?

Comment 10

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Vincent

(In reply to Vincent Liu[:vliu] from comment #8)
> > We already have separate system daemons for NFC and Bluetooth, a framework
> > for writing new daemons, and experience how to do it. Graphics would be a
> > welcome addition.
> 
> I am not sure new daemon architecture also fit for Graphics/Camera when we
> talk about performance. Currently I prefer building as .so file and
> manipulating methods by static pointer. Please see this bug for my idea.

True. Performance is one of the major concerns I had. Usually, buffer creation (e.g., opening windows) is not a problem, but the rendering and buffer swapping needs to be fast.

If you're building a shared library, can you put it in a separate Android repository? That would give us a clean architecture and we can later experiment with out-of-process graphics drivers without changing Gecko.

Another point is the interface definition: we need a header file that defines the interface between Gecko and your window library. We have a similar problem for Bluetooth configuration (bug 1188461) and Geolocation (bug 1183516). I intend to set up a new repository under 'hardware/libhardware_moz' that contains all our hardware-interfaces and similar headers. If you don't mind, you could put your header files there as well.

Comment 11

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8643578 [details] [diff] [review]
WIP-v3.patch

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

Hi Vincent,

I briefly read through the patch and left some comments about potential problems and things that break between Android/Gecko releases.

 * There's a overview of C++ binary compatibility at [1].
 * Maybe you could move the abstraction into |GonkNativeWindow|.

[1] https://techbase.kde.org/Policies/Binary_Compatibility_Issues_With_C++#The_Do.27s_and_Don.27ts

::: widget/gonk/hal/GonkBufferQueueHal/GonkBufferQueueHal.cpp
@@ +17,5 @@
> +
> +#include "GonkBufferQueueHal.h"
> +#if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 21
> +#include "GonkBufferQueueProducer.h"
> +#endif 

Trailing whitespace

@@ +54,5 @@
> +GonkBufferQueueHal::setBufferQueueSynchronousMode(bool aEnabled)
> +{
> +#if ANDROID_VERSION >= 21
> +  static_cast<GonkBufferQueueProducer*>(mProducer.get())->setSynchronousMode(aEnabled);
> +#elif ANDROID_VERSION >= 17  

Trailing whitespace

::: widget/gonk/hal/GonkBufferQueueHal/GonkBufferQueueHal.h
@@ +24,5 @@
> +#include "GonkBufferQueueProducer.h"
> +#endif
> +
> +namespace android {
> +class GonkBufferQueueHal : public GonkBufferQueueHalBase

Don't inherit and don't use 'virtual' if you don't need it. These things can make your abstraction break easily.

@@ +36,5 @@
> +    virtual sp<GonkNativeWindow>            getNativeWindow() override { return mNativeWindow; };
> +    virtual sp<IGraphicBufferProducer>      getBufferProducer() override;
> +    virtual void                            setBufferQueueSynchronousMode(bool aEnabled) override;
> +    virtual void                            setNewFrameCallback(GonkNativeWindowNewFrameCallback* callback) override;
> +    virtual already_AddRefed<mozilla::layers::TextureClient> getCurrentBuffer() override;

|already_AddRefed<>| is an inline-constructed object. This can become incompatible between Gecko releases.

This code also depends on |TextureClient|, which can also become incompatible between Gecko releases.

@@ +41,5 @@
> +    virtual void                            Clear() override;
> +    virtual void                            Abandon() override;
> +protected:
> +     int mBufferCount;
> +     sp<GonkNativeWindow>          mNativeWindow;

I'd also be careful with these shared pointers.

@@ +42,5 @@
> +    virtual void                            Abandon() override;
> +protected:
> +     int mBufferCount;
> +     sp<GonkNativeWindow>          mNativeWindow;
> +#if ANDROID_VERSION >= 21

You cannot use ANDROID_VERSION in your abstract interfaces.

Comment 12

[Vincent Liu[:vliu]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #11)
> Comment on attachment 8643578 [details] [diff] [review]
> WIP-v3.patch
> 
> Review of attachment 8643578 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Vincent,
> 
> I briefly read through the patch and left some comments about potential
> problems and things that break between Android/Gecko releases.
> 
>  * There's a overview of C++ binary compatibility at [1].
>  * Maybe you could move the abstraction into |GonkNativeWindow|.
> 
> [1]
> https://techbase.kde.org/Policies/
> Binary_Compatibility_Issues_With_C++#The_Do.27s_and_Don.27ts
> 

Hi Thomas,

Thanks for the Comment. I want to take some time to study [1] to think through the possible changes for my current WIP.

Comment 13

[Vincent Liu[:vliu]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #11)
> Comment on attachment 8643578 [details] [diff] [review]
> WIP-v3.patch
> 
> Review of attachment 8643578 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Vincent,
> 
> I briefly read through the patch and left some comments about potential
> problems and things that break between Android/Gecko releases.
> 
>  * There's a overview of C++ binary compatibility at [1].
>  * Maybe you could move the abstraction into |GonkNativeWindow|.
> 
> [1]
> https://techbase.kde.org/Policies/
> Binary_Compatibility_Issues_With_C++#The_Do.27s_and_Don.27ts
> 
> 
> ::: widget/gonk/hal/GonkBufferQueueHal/GonkBufferQueueHal.h
> @@ +24,5 @@
> > +#include "GonkBufferQueueProducer.h"
> > +#endif
> > +
> > +namespace android {
> > +class GonkBufferQueueHal : public GonkBufferQueueHalBase
> 
> Don't inherit and don't use 'virtual' if you don't need it. These things can
> make your abstraction break easily.
> 
> @@ +36,5 @@
> > +    virtual sp<GonkNativeWindow>            getNativeWindow() override { return mNativeWindow; };
> > +    virtual sp<IGraphicBufferProducer>      getBufferProducer() override;
> > +    virtual void                            setBufferQueueSynchronousMode(bool aEnabled) override;
> > +    virtual void                            setNewFrameCallback(GonkNativeWindowNewFrameCallback* callback) override;
> > +    virtual already_AddRefed<mozilla::layers::TextureClient> getCurrentBuffer() override;
> 

Hi Thomas,

Thanks for you feedback. I am not sure the meaning the Comment you gave above. Currently this base class is used as my interface exposing to Gecko and that's the reason I need it. Could you please have more comment about this? Thanks.

Comment 14

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Vincent,

I thought that everything in GonkBufferQueueHal.{cpp,h} will move into a shared library at some point. I was worried about the compatibility with future versions of Gecko. If all code remains within Gecko, you should be fine.

Regarding the use of a base class, I did not really understand why there's a base class if you only have one implementation. Since your current implementation seems to wrap |GonkNativeWindow|, you might just push everything into |GonkNativeWindow| itself.

But I don't know much about our graphics stack, nor do I have any say here. So take my advices with a grain of salt.

Comment 15

[u459114]

Comment on attachment 8643578 [details] [diff] [review]
WIP-v3.patch

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

::: widget/gonk/hal/GonkBufferQueueHal/GonkBufferQueueHal.h
@@ +24,5 @@
> +#include "GonkBufferQueueProducer.h"
> +#endif
> +
> +namespace android {
> +class GonkBufferQueueHal : public GonkBufferQueueHalBase

GonkBufferQueueHalBase is the abstraction while GonkBufferQueueHal is the concrete implementation.

@@ +42,5 @@
> +    virtual void                            Abandon() override;
> +protected:
> +     int mBufferCount;
> +     sp<GonkNativeWindow>          mNativeWindow;
> +#if ANDROID_VERSION >= 21

We cab have ANDROID_VERSION here, since this header is a private one.

Comment 16

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

Comment on attachment 8643578 [details] [diff] [review]
WIP-v3.patch

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

::: dom/camera/GonkCameraHwMgr.cpp
@@ +205,5 @@
>    mCamera->sendCommand(CAMERA_CMD_ENABLE_SHUTTER_SOUND, 0, 0);
>  
> +  mBufferQueueHal = GonkBufferQueueHalBase::CreateGonkBufferQueueHal();
> +
> +  if (mBufferQueueHal) {

MOZ_ASSERT(mBufferQueueHal) instead?

Comment 17

[Vincent Liu[:vliu]]

(In reply to John Lin [:jolin][:jhlin] from comment #16)
> Comment on attachment 8643578 [details] [diff] [review]
> WIP-v3.patch
> 
> Review of attachment 8643578 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/camera/GonkCameraHwMgr.cpp
> @@ +205,5 @@
> >    mCamera->sendCommand(CAMERA_CMD_ENABLE_SHUTTER_SOUND, 0, 0);
> >  
> > +  mBufferQueueHal = GonkBufferQueueHalBase::CreateGonkBufferQueueHal();
> > +
> > +  if (mBufferQueueHal) {
> 
> MOZ_ASSERT(mBufferQueueHal) instead?

Good suggestion. I will add it in the future patch. Thanks

Comment 18

[Vincent Liu[:vliu]]

Sorry to late response.

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #14)
> Hi Vincent,
> 
> I thought that everything in GonkBufferQueueHal.{cpp,h} will move into a
> shared library at some point. I was worried about the compatibility with
> future versions of Gecko. If all code remains within Gecko, you should be
> fine.

Do you mean the case about backward compatibility? I can image that the interface may be conflict once the interface may be different for future versions of Gecko. 

> 
> Regarding the use of a base class, I did not really understand why there's a
> base class if you only have one implementation. Since your current
> implementation seems to wrap |GonkNativeWindow|, you might just push
> everything into |GonkNativeWindow| itself.
> 

This base class is act as an interface exposing to Gecko to hide Android platform dependent part. There is no doubt that the interface may be different for the future release. Once the need of interface is changed, it is the timing to have a next version of Gonk release. The interface of this new version of Gonk release may include the original part and the change part. 

To solve compatibility, I think it should involve a way like XPCOM implementation to query which version of interface the Gecko faces before calling any method. For any method call from Gecko which Gonk doesn't support should know when querying instance. I am trying to study how to add this part in my current WIP. Please correct me if I got anything wrong.

Comment 19

[Vincent Liu[:vliu]]

Hi Thomas,

Could you please have some comments for Comment 18? Thanks.

Comment 20

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Vincent Liu[:vliu] from comment #19)
> Hi Thomas,
> 
> Could you please have some comments for Comment 18? Thanks.

Hi,

Generally speaking, you can do it like this:

  struct MozWindowInterface {

    /* function pointers here */

  };

  const struct MozWindowInterface
  moz_window_get_interface(uint32_t* aVersion);

If you have a shared library, the function |moz_window_get_interface| is its only officially exported symbol. It returns a pointer to the stucture with the actual interface and a version number. The structure contains function pointers for all operations, the version number tells you, which of these pointers are valid.

|GonkNativeWindow| would load and use this shared library to access HW operations. That's why I said that you could try to modify |GonkNativeWindow| instead of building a wrapper around it.

I don't know enough about Android's actual window code to give concrete advises on the API. Let me make up an API to illustrate my points. The interface structure could look like this

  #define MOZ_WINDOW_API_VERSION (2) /* We're building Gecko against this version */

  typedef void* MozWindow;

  struct MozWindowInterface {

    /* API version 1 */

    MozWindow (*create_window)();  /* creates a new window buffer */
    void (*ref_window)(MozWindow aWindow); /* incr window ref count */
    void (*unref_window)(MozWindow aWindow); /* decr window refcount */

    void (*swap)(MozWindow aWindow); /* buffer swapping */

    void (*flush)(MozWindow aWindow); /* flush rendering commands */

    /* API version 2 */

    void (*sync)(MozWindow aWindow); /* flush and wait for completion */
  };

MozWindow is simply a void* and its definition is hidden inside the shared library. So nothing in Gecko depends on its size or fields. The first three functions manage window references (creation and ref counting). The ref/unref functions hide the details of Android's shared pointers (|sp<>|), which might change between releases.

The example API has two versions, which |sync| only being available in version 2. The shared libraries maximum supported version is returned in |aWindow| in |moz_window_get_interface|. If Gecko has been compiled against version 2 of the API, it can call |sync|. For older shared libraries that only support version 1, Gecko would know that |sync| is not available.

As long as you only add function pointers to the end of the interface structure, binary compatibility we be ensured among different versions.

In the end, Gecko would contain one implementation of |GonkNativeWindow| that manages the libraries and handles the differences between the versions. Coming back to |sync|, a sync method in |GonkNativeWindow| could look like this.

  void GonkNativeWindow::Sync()
  {
  #if MOZ_WINDOW_API_VERSION >= 2 // We support API version 2 in Gecko
    if (aVersion >= 2) { // The shared library also supports version 2
      aInterface->sync(); // Let's call |sync|
    } else
  #else
    {
      aInterface->flush(); // We call |flush| if sync is not supported by Gecko or the lib
    }
  #endif
  }