Closed Bug 1133007 Opened 7 years ago Closed 7 years ago

(LayerScope) Prevent sending duplicate texture image

Categories

(Core :: Graphics, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: u459114, Assigned: u459114)

References

Details

Attachments

(2 files, 20 obsolete files)

45 bytes, text/x-github-pull-request
boris
: review+
Details | Review
24.09 KB, patch
u459114
: review+
Details | Diff | Splinter Review
LayerScope read image of every texture of every layer.
1. ReadPixel of each texture spends tons of time.
2. Sent the same image via websocket is really slow, and the viewer need to process all duplicate images, even slower...

Prevent sending duplicate image should be a big perf win for layerscope
Attachment #8564302 - Attachment is obsolete: true
Comment on attachment 8564303 [details] [diff] [review]
Prevent sending duplicate texture images

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

Hi Dan,
Please help reviewing this patch.

By this change, we don't repeat sent the same image via webseocket to the viewer. Two benefits at device side
1. Reduce total amount of ReadPixel
2. Reduce socket traffic 
For viewer side, we don't need to hash image again. Instead, we can leverage texture.contentid as hash key.

LayerScope will not reduce device FPS so much after this change.
Attachment #8564303 - Flags: review?(dglastonbury)
Attached file viewer side change (obsolete) —
Attachment #8564316 - Flags: review?(mtseng)
Attachment #8564316 - Flags: review?(boris.chiou)
Attachment #8564303 - Flags: review?(dglastonbury) → review+
Attachment #8564303 - Flags: review+ → review?(chung)
Attachment #8564316 - Flags: review?(mtseng) → review+
Comment on attachment 8564303 [details] [diff] [review]
Prevent sending duplicate texture images

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

Looks good to me but some part seems too complicated than needed.

::: gfx/layers/LayerScope.cpp
@@ +350,4 @@
>  
>      // Packet data
>      Packet mPacket;
> +    uint32_t  mContentUUID;

Why not use serial number on Texture directly?

@@ +439,4 @@
>      bool mComposedByHwc;
>  };
>  
> +class ImageContentCache {

Given the serial number of TextureHost is strictly increasing, we only need to tracking the last available serial for each texture.

In that case, a hash maps TextureSourceOGL* to uint32_t should be fine and effective than a linear search here. (This map should be a member of a debug session if we have debug session abstraction.)

@@ +506,5 @@
> +
> +private:
> +    static StaticAutoPtr<ImageContentCache> sContentCache;
> +
> +    uint32_t mContentUUID;

What is this UUID for?
Attachment #8564303 - Flags: review?(chung) → review-
Attachment #8564303 - Attachment is obsolete: true
Attachment #8564893 - Flags: review?(chung)
Attachment #8564893 - Flags: review?(chung) → review+
Comment on attachment 8564316 [details]
viewer side change

LGTM. Only https://github.com/CJKu/layerscope/pull/1/files#diff-e2874a901f7b01f94f61d645c612e572R318 needs to be fixed as we discussed offline.
Attachment #8564316 - Flags: review?(boris.chiou) → review+
Full try since touched a low-level gfx class - TexturedEffect..

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c1633edbdcd
Oop(In reply to C.J. Ku[:cjku] from comment #8)
> Full try since touched a low-level gfx class - TexturedEffect..
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c1633edbdcd

Oops... wrong place
Attachment #8612370 - Attachment is obsolete: true
Attachment #8612371 - Attachment is obsolete: true
Comment on attachment 8612377 [details] [diff] [review]
Prevent duplicate texture package

Hi Morris, here is a patch depend on what we discussed yesterday. Please have a look, thanks
Attachment #8612377 - Flags: review?(mtseng)
Attached file View size change
Attachment #8564316 - Attachment is obsolete: true
Attachment #8564893 - Attachment is obsolete: true
Attachment #8612393 - Flags: review?(mtseng)
Attachment #8612393 - Flags: review?(boris.chiou)
Attachment #8612377 - Flags: review?(boris.chiou)
Attachment #8612406 - Flags: review?(mtseng)
Attachment #8612404 - Flags: review?(boris.chiou)
Attachment #8612377 - Attachment is obsolete: true
Attachment #8612377 - Flags: review?(mtseng)
Attachment #8612377 - Flags: review?(boris.chiou)
Attachment #8612404 - Attachment is obsolete: true
Attachment #8612404 - Flags: review?(boris.chiou)
Attachment #8612417 - Flags: review?(boris.chiou)
Attachment #8612406 - Attachment is obsolete: true
Attachment #8612406 - Flags: review?(mtseng)
Attachment #8612418 - Flags: review?(mtseng)
Attachment #8612417 - Attachment is obsolete: true
Attachment #8612418 - Attachment is obsolete: true
Attachment #8612417 - Flags: review?(boris.chiou)
Attachment #8612418 - Flags: review?(mtseng)
Comment on attachment 8612699 [details] [diff] [review]
Create a singlton holder to put all singleton in

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

Hi Dan/Boris,
I add a new singleton object, access via Singletons::Instnace(), to hold all singletons object in layer scope. So, ideally, there should be only one singleton in LayerScope.
To keep as less singleton as possible is the original thought.
Attachment #8612699 - Flags: review?(dglastonbury)
Attachment #8612699 - Flags: review?(boris.chiou)
Comment on attachment 8612700 [details] [diff] [review]
Send texture content only if it was altered

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

Hi Dan/Morris,
On B2G, Since GrallocTextureHost is not a DataTextureSource, Attachment 8564893 [details] [diff] is not really useful on B2G.
In this new patch, TextureHost will notify LayerScope that content been updated in TextureHost::Updated. So that LayerScope knows it need to re-send the content of that texture again since it's altered.
The FPS boosts up from 1FPS to 30FPS after apply this change.
  
Please help to review.
Attachment #8612700 - Flags: review?(mtseng)
Attachment #8612700 - Flags: review?(dglastonbury)
Comment on attachment 8612699 [details] [diff] [review]
Create a singlton holder to put all singleton in

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

::: gfx/layers/LayerScope.cpp
@@ +193,5 @@
> +    gfx::Rect mLayerRects[4];
> +};
> +
> +// Hold all singleton objects used by LayerScope
> +class Singletons

I think we can use a better class name, ex. LayerScopeManager or others.

@@ +242,2 @@
>      {
>          return sWebSocketManager;

Redundant code

@@ +254,3 @@
>  private:
> +    //   Hide constructor.
> +    Singletons(){ }

You can use "Singleton() = delete;"

@@ +935,4 @@
>      }
>  
>      // Transfer ownership to SocketManager.
> +    Singletons::Instance()->GetSocketManager()->AppendDebugData(package.release());

indent

@@ +1645,5 @@
>              new DebugGLMetaData(Packet::META, true));
>      }
>  }
>  
> +void

typo ?
Comment on attachment 8612699 [details] [diff] [review]
Create a singlton holder to put all singleton in

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

::: gfx/layers/LayerScope.cpp
@@ +935,4 @@
>      }
>  
>      // Transfer ownership to SocketManager.
> +    Singletons::Instance()->GetSocketManager()->AppendDebugData(package.release());

Sorry. Please ignore this comment.
Comment on attachment 8612699 [details] [diff] [review]
Create a singlton holder to put all singleton in

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

::: gfx/layers/LayerScope.cpp
@@ +254,3 @@
>  private:
> +    //   Hide constructor.
> +    Singletons(){ }

Sorry. You still need this constructor, so please ignore this comment.
Attachment #8612699 - Attachment is obsolete: true
Attachment #8612699 - Flags: review?(dglastonbury)
Attachment #8612699 - Flags: review?(boris.chiou)
Comment on attachment 8612818 [details] [diff] [review]
Create a singlton holder to put all singleton objects in

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

Fixed what boris mentioned
Attachment #8612818 - Flags: review?(dglastonbury)
Attachment #8612818 - Flags: review?(boris.chiou)
rebase
Attachment #8612700 - Attachment is obsolete: true
Attachment #8612700 - Flags: review?(mtseng)
Attachment #8612700 - Flags: review?(dglastonbury)
Attachment #8612823 - Flags: review?(dglastonbury)
Attachment #8612823 - Flags: review?(boris.chiou)
Attachment #8612823 - Flags: review?(boris.chiou) → review?(mtseng)
Comment on attachment 8612818 [details] [diff] [review]
Create a singlton holder to put all singleton objects in

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

::: gfx/layers/LayerScope.cpp
@@ +181,5 @@
> +class DrawSession {
> +public:
> +    DrawSession()
> +      : mOffsetX(0.0),
> +        mOffsetY(0.0),

nit: Format like this:

    DrawSession()
      : mOffsetX(0.0)
      , mOffsetY(0.0)
      , mRects(0)
    { }

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes

@@ +198,5 @@
>  {
>  public:
> +    static LayerScopeManager* Instance() {
> +        if (!sLayerScopeManager) {
> +            sLayerScopeManager = new LayerScopeManager();

The problem with this style of singleton is that we lose the ability to control when to deallocate the LayerScopeManager object. Is a static instance method really any better than having a global object `gLayerScopeManager`? (I don't think it is)

@@ +301,5 @@
>  
>          uint32_t size = aPacket.ByteSize();
>          auto data = MakeUnique<uint8_t[]>(size);
>          aPacket.SerializeToArray(data.get(), size);
> +        return LayerScopeManager::Instance()->GetSocketManager()->WriteAll(data.get(), size);

Stylistically, I really dislike this list of pointer derefs.

@@ +646,4 @@
>              return NS_OK;
>  
>          printf_stderr("*** LayerScope: Accepted connection\n");
> +        LayerScopeManager::Instance()->GetSocketManager()->AddConnection(aTransport);

Ditto.
Comment on attachment 8612823 [details] [diff] [review]
Send texture content only if it was altered

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

r+ with nits fixed.

::: gfx/layers/LayerScope.cpp
@@ +203,5 @@
> +            mChangedHosts.AppendElement(host);
> +        }
> +    }
> +
> +    void Clear(const TextureHost* host) {

Clear sounds too much like Empty. Or rename using something more descriptive, such `RemoveChangedHost`.

@@ +210,5 @@
> +        }
> +    }
> +
> +    bool IsChanged(const TextureHost* host) {
> +        if (THArray::NoIndex == mAllHosts.IndexOf(host)) {

Is the point of AllHosts array that all texture hosts are marked as changed at least one? If so, I suggest changing `All` for a less generic name. Perhaps `mSeenHosts`. Also add a comment explaining why `IsChanged` returns true if hosts is not in the `mSeenHosts` array.

@@ +215,5 @@
> +            mAllHosts.AppendElement(host);
> +            return true;
> +        }
> +
> +        if (decltype(mChangedHosts)::NoIndex != mChangedHosts.IndexOf(host)) {

In the previous if, you used `THArray::NoIndex` and in this one `decltype(mChangedHosts)::NoIndex`.  Please be consistent.

Also the if and return false can be eliminated:

    return mChangedHosts.IndexOf(host) != THArray::NoIndex;

@@ +436,5 @@
> +        if (packData) {
> +            uint8_t* grallocData = nullptr;
> +            if (BAD_VALUE == buffer->lock(GRALLOC_USAGE_SW_READ_OFTEN |
> +                                           GRALLOC_USAGE_SW_WRITE_NEVER,
> +                                           reinterpret_cast<void**>(&grallocData))) {

When in if statement has multiple lines, place the { at the start of the block, instead on on the end-of-line:

    if (BAD_VALUE == buffer->lock(GRALLOC_USAGE_SW_READ_OFTEN |
                                       GRALLOC_USAGE_SW_WRITE_NEVER,
	                               reinterpret_cast<void**>(&grallocData)))
    {
        return false;
    }

If the condition fits on a single-line, keep the { at the end:

    if (simple) {
        return true;
    }

::: gfx/layers/LayerScope.h
@@ +44,1 @@
>  private:

Empty line before `private:`
Attachment #8612823 - Flags: review?(dglastonbury) → review+
Attachment #8612818 - Attachment is obsolete: true
Attachment #8612823 - Attachment is obsolete: true
Attachment #8612818 - Flags: review?(dglastonbury)
Attachment #8612818 - Flags: review?(boris.chiou)
Attachment #8612823 - Flags: review?(mtseng)
Attachment #8613356 - Flags: review+
Attachment #8612393 - Flags: review?(boris.chiou) → review+
Hi Dan,
This patch fixed problems that you mentioned in the review, please review again, thank.
Attachment #8613355 - Attachment is obsolete: true
Attachment #8613373 - Flags: review?(dglastonbury)
Attachment #8613373 - Flags: review?(boris.chiou)
Attachment #8613356 - Attachment is obsolete: true
Attachment #8613375 - Flags: review+
Comment on attachment 8613373 [details] [diff] [review]
Create a singlton holder to put all singleton in

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

LGTM. Just wait for Dan's review
Attachment #8613373 - Flags: review?(boris.chiou) → review+
Merged two patches and submitted a try
Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=79d2257be5a3
Attachment #8613375 - Attachment is obsolete: true
Attachment #8613461 - Flags: review+
Fix build break on windows and push to try again.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b892ab13bd7b
Comment on attachment 8613373 [details] [diff] [review]
Create a singlton holder to put all singleton in

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

LGTM.
Attachment #8613373 - Flags: review?(dglastonbury) → review+
Attached patch Check-in patchSplinter Review
Combine two patches.
Attachment #8613373 - Attachment is obsolete: true
Attachment #8613461 - Attachment is obsolete: true
Attachment #8614025 - Flags: review+
Keywords: checkin-needed
Blocks: LayerScope
No longer depends on: LayerScope
You need to log in before you can comment on or make changes to this bug.