Closed Bug 1254011 Opened 4 years ago Closed 3 years ago

Avoid allocating RGB buffer for YUV data everytime.

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: jrmuizel, Assigned: sotaro)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files, 12 obsolete files)

9.80 KB, patch
nical
: review+
Details | Diff | Splinter Review
18.51 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
Allocations can be expensive.
Assignee: nobody → sotaro.ikeda.g
Assignee: sotaro.ikeda.g → nobody
Whiteboard: [gfx-noted]
If Bug 1254006 is addressed, we could avoid allocating RGB buffer within BufferTextureHost.
FWIW, I'd like to avoid having more than one of these RGB buffers around. On lower end machines I've seen lots of memory used on fullscreen youtube so I'd like to minimize things as much as possible. I think the right place to retain this buffer is probably on the ImageLayer.
So we currently do the conversion in DataSourceSurfaceFromYCbCrDescriptor called from BufferTextureHost::Upload. I think it may make sense to move this out of upload. However, moving it into DrawQuad isn't great either because we don't have access to the ImageLayer there.

FWIW, we also currently do a conversion from unpremultiplied data to premultiplied data in DrawQuad and there's a comment about caching the result.
Attachment #8767164 - Attachment is obsolete: true
Assignee: nobody → sotaro.ikeda.g
Attachment #8767166 - Attachment is obsolete: true
Attachment #8767536 - Attachment is obsolete: true
Attachment #8767565 - Attachment is obsolete: true
I re-checked fps with current master, since Bug 1254011 is progressed as "Avoid allocating RGB buffer for YUV data everytime". Fps was a bit better than before Bug 1280839 seemed to improve it.

Compared fullscreen youtube playback performance between "master + Bug 1254011" and "master + bug 1254010 + attachment 8767059 [details] [diff] [review]" by using the following video on my laptop.

 - https://www.youtube.com/watch?v=iNJdPyoqt8U (30fps)

- "master + bug 1254010 + attachment 8761895 [details] [diff] [review] [diff] [review]" (scaling and conversion once if possible)
  + 2160p(4K): 12 - 30 fps (fps was unstable)
  + 1440p    : 24 fps
  + 1080p    : 30 fps
  + 720p     : 30 fps
  + 480p     : 30 fps

- "master + Bug 1254011" (with recycle buffer and defer YUV to RGB conversion)
  + 2160p(4K): 8 - 23 fps (fps was unstable)
  + 1440p    : 18 fps
  + 1080p    : 24 fps
  + 720p     : 29 fps
  + 480p     : 30 fps

- "master"
  + 2160p(4K): 6 - 16 fps (fps was unstable)
  + 1440p    : 15 fps
  + 1080p    : 21 fps
  + 720p     : 28 fps
  + 480p     : 30 fps
Attachment #8767569 - Flags: feedback?(nical.bugzilla)
Attachment #8767573 - Flags: feedback?(nical.bugzilla)
Comment on attachment 8767569 [details] [diff] [review]
patch part 1 - Defer YUV conversion

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

::: gfx/layers/basic/BasicCompositor.cpp
@@ +80,5 @@
>    RefPtr<gfx::DataSourceSurface> mSurface;
>    bool mWrappingExistingData;
>  };
>  
> +class DataTextureSourceYCbCrBasic : public DataTextureSource

Please add a comment explaining the purpose of this class. It took me a while to figure out that it was in order to lazily do the yuv->rgb conversion when GetSurface is called which happens in BasicCompositor::DrawQuad rather than when we receive a transaction!

Also, (nit) I think that WrappingTextureSourceYCbCrBasic would be a better name since we don't really implement the DataTextureSource interface on it.
Attachment #8767569 - Flags: feedback?(nical.bugzilla) → feedback+
Comment on attachment 8767573 [details] [diff] [review]
patch part 2 - Add DataSourceSurfaceRecycleAllocator

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

I think that you could achieve the same result without adding a new Recycling abstraction, by doing something like what ImageHost::SetCurrentTextureHost does: Detect that the current texture source is reusable by the next TextureHost and mark it as such (SetOwner(nullptr)), so that the next BufferTextureHost can decide (in PrepareTextureSource, maybe?) to reuse it next time. This could be achieved in less code, and since it would be using an already existing mechanism it would not add extra complexity, so I'd prefer that you do it that way.
Attachment #8767573 - Flags: feedback?(nical.bugzilla) → feedback-
Attachment #8767569 - Attachment is obsolete: true
Attachment #8767841 - Attachment is obsolete: true
Apply the comment.
Attachment #8767573 - Attachment is obsolete: true
Comment on attachment 8767874 [details] [diff] [review]
patch part 2 - Reuse WrappingTextureSourceYCbCrBasic

:nical, how about this patch?
Attachment #8767874 - Flags: feedback?(nical.bugzilla)
Comment on attachment 8767874 [details] [diff] [review]
patch part 2 - Reuse WrappingTextureSourceYCbCrBasic

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

I like this a lot more, thanks!

::: gfx/layers/basic/BasicCompositor.cpp
@@ +92,5 @@
>    virtual const char* Name() const override { return "WrappingTextureSourceYCbCrBasic"; }
>  
>    explicit WrappingTextureSourceYCbCrBasic(BufferTextureHost* aTexture)
>    : mTexture(aTexture)
> +  , mNeedsUpdate(true)

nit: You could remove mNeedsUpdate and just clear mTexture when the update is done mTexture != null would mean that an update is needed (not very important).
Attachment #8767874 - Flags: feedback?(nical.bugzilla) → feedback+
(In reply to Nicolas Silva [:nical] from comment #17)
> 
> nit: You could remove mNeedsUpdate and just clear mTexture when the update
> is done mTexture != null would mean that an update is needed (not very
> important).

Just removing mNeedsUpdate could cause a problem to WrappingTextureSourceYCbCrBasic::Unbind() in current code. It seems simpler just to keep mNeedsUpdate.
Add some asserts.
Attachment #8767874 - Attachment is obsolete: true
Fix a warning.
Attachment #8767856 - Attachment is obsolete: true
Attachment #8767903 - Attachment is obsolete: true
Attachment #8768263 - Flags: review?(nical.bugzilla)
Attachment #8768264 - Flags: review?(nical.bugzilla)
Comment on attachment 8768263 [details] [diff] [review]
patch part 1 - Defer YUV conversion

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

Sorry for the review lag.
Attachment #8768263 - Flags: review?(nical.bugzilla) → review+
Attachment #8768264 - Flags: review?(nical.bugzilla) → review+
Rebased
Attachment #8768263 - Attachment is obsolete: true
Attachment #8769404 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ff1dc7f8a1d0
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
This patch did a great perf improvement on our video talos test, thanks!
https://treeherder.mozilla.org/perf.html#/alerts?id=1777
Depends on: 1304999
You need to log in before you can comment on or make changes to this bug.