Cleanup ImageContainerChild

RESOLVED FIXED in mozilla19

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: nical, Assigned: nical)

Tracking

Trunk
mozilla19
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
There is some code duplication that we should get rid of, by slightly reorganizing the code in ImageContainerChild.cpp.
(Assignee)

Comment 1

6 years ago
Created attachment 662328 [details] [diff] [review]
Reorganize the async-video code to avoid code duplication

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)
(Assignee)

Updated

6 years ago
Blocks: 773440
(Assignee)

Updated

6 years ago
Blocks: 783366
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

6 years ago
Created attachment 663028 [details] [diff] [review]
Reorganize the async-video code to avoid code duplication

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 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

6 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

6 years ago
Created attachment 676177 [details] [diff] [review]
Reorganize the async-video code to avoid code duplication

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

6 years ago
Keywords: checkin-needed
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/42b38e4e9068
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19

Updated

6 years ago
Blocks: 823236

Updated

6 years ago
No longer blocks: 823236
Depends on: 823236
You need to log in before you can comment on or make changes to this bug.