Closed
Bug 792252
Opened 12 years ago
Closed 12 years ago
Cleanup ImageContainerChild
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: nical, Assigned: nical)
References
Details
Attachments
(1 file, 2 obsolete files)
15.73 KB,
patch
|
Details | Diff | Splinter Review |
There is some code duplication that we should get rid of, by slightly reorganizing the code in ImageContainerChild.cpp.
Assignee | ||
Comment 1•12 years ago
|
||
With this patch the logic of copying into the the shared image (shmem) is moved to ShmemYCbCrImage rather than copy-pasting the same loops all over the place.
the code to send images to the compositor is reorganized so that we have clear steps:
- try to convert the image to a shared image without need for copy
- try to get a shared image without allocating (if needed)
- try to allocate a shared image (if needed)
- copy in the image (if needed)
- send the image
rather than having two code paths that do the copy as we do currently.
It works and passes try, but i still need to make sure i did not break b2g.
Assignee: nobody → nsilva
Attachment #662328 -
Flags: review?(jones.chris.g)
Comment 2•12 years ago
|
||
Comment on attachment 662328 [details] [diff] [review]
Reorganize the async-video code to avoid code duplication
Review of attachment 662328 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/ipc/ImageContainerChild.cpp
@@ +302,5 @@
> + img = AllocateSharedImageFor(aImage);
> + }
> + }
> +
> + if (img && CopyDataIntoSharedImage(aImage, img)) {
CopyDataIntoSharedImage shouldn't be checked if the image could be converted to a shared image directly.
Assignee | ||
Comment 3•12 years ago
|
||
Aw, bad mistake, fixed.
Attachment #662328 -
Attachment is obsolete: true
Attachment #662328 -
Flags: review?(jones.chris.g)
Attachment #663028 -
Flags: review?(jones.chris.g)
Comment on attachment 663028 [details] [diff] [review]
Reorganize the async-video code to avoid code duplication
Good grief, sorry about this review lag! This dropped off my plate while we were rushing around for the aurora uplift :(. Feel free to make more annoying pings of me ;).
Kan-Ru's review is sufficient for this.
Attachment #663028 -
Flags: review?(jones.chris.g) → review?(kchen)
Comment 5•12 years ago
|
||
Comment on attachment 663028 [details] [diff] [review]
Reorganize the async-video code to avoid code duplication
Review of attachment 663028 [details] [diff] [review]:
-----------------------------------------------------------------
LGTM
::: gfx/layers/ipc/ImageContainerChild.cpp
@@ +289,5 @@
> + return;
> + }
> +
> + NS_ABORT_IF_FALSE(InImageBridgeChildThread(),
> + "Should be in ImageBridgeChild thread.");
already checked?
Attachment #663028 -
Flags: review?(kchen) → review+
Assignee | ||
Comment 6•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> Comment on attachment 663028 [details] [diff] [review]
> Reorganize the async-video code to avoid code duplication
>
> Good grief, sorry about this review lag! This dropped off my plate while we
> were rushing around for the aurora uplift :(. Feel free to make more
> annoying pings of me ;).
>
> Kan-Ru's review is sufficient for this.
No problem, I am in vacations so I was not in a rush.
(In reply to Kan-Ru Chen [:kanru] from comment #5)
> Comment on attachment 663028 [details] [diff] [review]
> Reorganize the async-video code to avoid code duplication
>
> Review of attachment 663028 [details] [diff] [review]:
> already checked?
Ha, indeed, i just fixed it and i will land the patch now.
Assignee | ||
Comment 7•12 years ago
|
||
sadly it looks like i don't have commit access any more.
can someone land this patch for me?
Attachment #663028 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 8•12 years ago
|
||
Comment on attachment 676177 [details] [diff] [review]
Reorganize the async-video code to avoid code duplication
># HG changeset patch
># User Nicolas Silva <nical.bugzilla@gmail.com>
># Date 1351523286 -3600
># Node ID 725e79a351acbb9ca532efd308139b2bc1f11770
># Parent 43c585774a134e99b359ed9c31a9e978ed0f7df0
>Bug 792252 - Clean up ImageContainerChild (before implementing SharedPlanarYCbCrImage to remove the extra video frame copy). r=kanru
>
>diff --git a/gfx/layers/ipc/ImageContainerChild.cpp b/gfx/layers/ipc/ImageContainerChild.cpp
>--- a/gfx/layers/ipc/ImageContainerChild.cpp
>+++ b/gfx/layers/ipc/ImageContainerChild.cpp
>@@ -1,24 +1,26 @@
>-/* -*- Mode: C++; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 2 -*-
>+ /* -*- Mode: C++; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 2 -*-
^^
I reverted this accidental-whitespace-introduction...
>@@ -55,17 +57,17 @@ ImageContainerChild::~ImageContainerChil
> MOZ_COUNT_DTOR(ImageContainerChild);
> }
>
> void ImageContainerChild::DispatchStop()
> {
> GetMessageLoop()->PostTask(FROM_HERE,
> NewRunnableMethod(this, &ImageContainerChild::StopChildAndParent));
> }
>-
>+
^^
...and this one, and landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/42b38e4e9068
Updated•12 years ago
|
Keywords: checkin-needed
Comment 9•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Updated•12 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•