792252 - Cleanup ImageContainerChild

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

There is some code duplication that we should get rid of, by slightly reorganizing the code in ImageContainerChild.cpp.

Comment 1

[Nicolas Silva [:nical]]

Attachment: 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.

Comment 2

[Kan-Ru Chen [:kanru] (UTC+9)]

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.

Comment 3

[Nicolas Silva [:nical]]

Attachment: Reorganize the async-video code to avoid code duplication

Aw, bad mistake, fixed.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 5

[Kan-Ru Chen [:kanru] (UTC+9)]

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?

Comment 6

[Nicolas Silva [:nical]]

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

Comment 7

[Nicolas Silva [:nical]]

Attachment: 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?

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 676177 [details] [diff] [review]
Reorganize the async-video code to avoid code duplication

># HG changeset patch
># User Nicolas Silva <[e-mail]>
># 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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/42b38e4e9068