Closed Bug 1049296 Opened 10 years ago Closed 9 years ago

[Stingray] Support sideband stream in HWComposer2D

Categories

(Core :: Graphics: Layers, defect)

Product:
Core
Component:
Graphics: Layers
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla46
Tracking Flags:
Tracking Status
firefox46 --- fixed

People

(Reporter: pchang, Assigned: sotaro)

References

Details

(Whiteboard: [ft:conndevices])

Attachments

(1 file, 30 obsolete files)

patch - Handle Sideband stream compositing in HwcComposer2D
(deleted), patch
sotaro
: review+
Details | Diff | Splinter Review
In bug 1002823, it tries to create a new kind image layer, OverlayImage which contains layer size and source information. Here we propose to reuse HWComposer2D to pass these information to driver. Since we are using HWC HAL to pass information to driver, we may have the capability with different HWC versions. In this bug, we also want to support the backward capability.
Depends on: 1002823
Assignee: nobody → boris.chiou
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
In this patch, we only handle normal case: All layers are composed by the hwccomposer. (No HWC_FRAMEBUFFER) 1. Check OverlayImage in HwcComposer2D by mImageSource (int32_t) in LayerRenderState. 2. OverlayImage should be set into the hwccomposer, and be composed by GPU (PS. GPU should draw the borders of this layer). BTW, please ignore these printf_stderrs, I will remove them in my new patch.
Attachment #8469929 - Attachment is obsolete: true
Attachment #8470747 - Flags: feedback?(chung)
Attachment #8470747 - Flags: feedback?(pchang)
Comment on attachment 8470747 [details] [diff] [review] [WIP] Support OverlayImage in HWComposer2D (v1.0) Review of attachment 8470747 [details] [diff] [review]: ----------------------------------------------------------------- I am thinking to add another TryHwCompositionforOverlayImage to append this func after TryHWComposition. In that func, we only send the OvelayImage layer into HWC. Then we don't care the logic inside TryHWComposition. ::: widget/gonk/HwcComposer2D.cpp @@ +119,4 @@ > } else { > mColorFill = false; > mRBSwapSupport = false; > + mOverlayImageSupport = false; No need this setup because mOverlayImageSupport is false by default. @@ +125,5 @@ > char propValue[PROPERTY_VALUE_MAX]; > property_get("ro.display.colorfill", propValue, "0"); > mColorFill = (atoi(propValue) == 1) ? true : false; > mRBSwapSupport = true; > + mOverlayImageSupport = false; same as above @@ +506,5 @@ > + if (mOverlayImageSupport && (state.mImageSource != 0)) { > + printf_stderr("[Boris] %s Layer has mImageSource %d", aLayer->Name(), state.mImageSource); > + hwcLayer.reserved[0] = state.mImageSource; > + } > +#endif I would prefer we support OverlayImage after android 19. @@ +592,5 @@ > + // Overlay Composition, set layer composition flag > + // on mapped LayerComposite to skip GPU composition > + mHwcLayerMap[k]->SetLayerComposited(true); > + } > + Here means fail to do full overlay composition because overlayComposite is false. I think you may also need to add overlayImage for full overlay composition. Maybe set overlayImageComposite in above for loop (line 560).
Attachment #8470747 - Flags: feedback?(pchang)
Here are some of my problems about implementation details. 1. Where should I put the mOverlayId (int32_t)? a) Now I put them into hwcLayer->handle (buffer_handle_t). However, I have to do many allocations/deletions and manage the memory resources correctly. b) The second option is to put them into the reserved fields, (hwcLayer->reserved[0]), but it may depend on Android version. c) Other fields. hwcLayer has many fields. If I am a OverlayImage, maybe some fields can be used. 2. According to current implementation of HwcComposer2D, we don't really support partial HW composition. If there is a HWC_FRAMEBUFFER, we will always do a full GPU composition, instead of a partial GPU composition. Therefore, I only can handle these two cases: a) All HWC_OVERLAY layers b) Some are HWC_BLIT and others are HWC_OVERLAY layers BTW, should I add a new compositionType (ex. HWC_OVERLAYIMAGE) for OverlayImage layer? 3. According to current implementation, we always call mHwc->set() for all layers together. Could we call mHwc->set() only for overlayImage layers? As Peter mentioned, maybe we can add a new function, TryHwCompositionforOverlayImage(), and append it after TryHWComposition(). In that function, we only send the OvelayImage layer into HWC.
Target at v2.2 with bug 1002823 to cover the whole user story.
feature-b2g: --- → 2.2
Whiteboard: [FT:Stream3]
(In reply to Boris Chiou [:boris] from comment #8) > Here are some of my problems about implementation details. > 1. Where should I put the mOverlayId (int32_t)? > a) Now I put them into hwcLayer->handle (buffer_handle_t). However, I > have to > do many allocations/deletions and manage the memory resources > correctly. > b) The second option is to put them into the reserved fields, > (hwcLayer->reserved[0]), but it may depend on Android version. > c) Other fields. hwcLayer has many fields. If I am a OverlayImage, maybe > some > fields can be used. > > 2. According to current implementation of HwcComposer2D, we don't really > support > partial HW composition. If there is a HWC_FRAMEBUFFER, we will always do > a full > GPU composition, instead of a partial GPU composition. Therefore, I only > can > handle these two cases: > a) All HWC_OVERLAY layers > b) Some are HWC_BLIT and others are HWC_OVERLAY layers > BTW, should I add a new compositionType (ex. HWC_OVERLAYIMAGE) for > OverlayImage > layer? > > 3. According to current implementation, we always call mHwc->set() for all > layers > together. Could we call mHwc->set() only for overlayImage layers? > As Peter mentioned, maybe we can add a new function, > TryHwCompositionforOverlayImage(), and append it after TryHWComposition(). > In that function, we only send the OvelayImage layer into HWC. Marco, the first question is related to how to save the source id in android HWC structure. The second/third questions are related to how do we pass the source id to kernel. We could combine passing source id with current HWC ioctl calls but we could also separate to another ioctl call. In my opinion, third one is better to me because we could have separate flow to handle HWOverlayImage.
Flags: needinfo?(mchen)
Status: NEW → ASSIGNED
(In reply to peter chang[:pchang][:peter] from comment #10) > Marco, the first question is related to how to save the source id in android > HWC structure. > As discussed with Peter offline. These questions should be the call made by designer of this feature and module owner/peer. Once you designed it then we can cooperate with TV partner to realize it.
Flags: needinfo?(mchen)
Use feature-b2g:2.2? rather than just 2.2.
feature-b2g: 2.2 → 2.2?
Flags: needinfo?(ying.xu)
Flags: needinfo?(ying.xu)
Comment on attachment 8478160 [details] [diff] [review] [WIP] Support OverlayImage in HWComposer2D (v2.1) Review of attachment 8478160 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gonk/HwcComposer2D.cpp @@ +381,5 @@ > + tempHandle->data[0] = static_cast<int>(state.mOverlayId); > + mOverlayIdMap[current] = tempHandle; > + > + hwcLayer.handle = tempHandle; > + hwcLayer.compositionType = HWC_OVERLAY; Boris, in order not to break android spec in [1], please keep the composition type as HWC_FRAMEBUFFER for OverlayImage case. I would suggest to add HWC_OVERLAYIMAGE in [2] to inform HAL that there is an OverlayImage layer coming. [1]https://github.com/android/platform_hardware_libhardware/blob/master/include/hardware/hwcomposer.h#L119 [2]http://dxr.mozilla.org/mozilla-central/source/widget/gonk/HwcUtils.h#44
NI for comment 16.
Flags: needinfo?(boris.chiou)
Comment on attachment 8500917 [details] [diff] [review] [WIP] Support OverlayImage in HWComposer2D (v2.2) Review of attachment 8500917 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gonk/HwcComposer2D.cpp @@ +522,5 @@ > + mOverlayIdMap[current] = tempHandle; > + > + hwcLayer.handle = tempHandle; > + hwcLayer.flags = HwcUtils::HWC_OVERLAYIMAGE; > + hwcLayer.compositionType = HWC_FRAMEBUFFER; OK, we always set compositionType as HWC_FRAMEBUFFER, and also set the flag, HWC_OVERLAYIMAGE, to notify the hwccomposer that this is an overlay image layer. However, I still put the overlay id into hwcLayer.handle in this patch.
Flags: needinfo?(boris.chiou)
Attachment #8502236 - Flags: feedback?(sushilchauhan)
Comment on attachment 8502236 [details] [diff] [review] [WIP] Support OverlayImage in HWComposer2D (v2.3) Review of attachment 8502236 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gonk/HwcComposer2D.cpp @@ +520,5 @@ > + memset((void*)(&tempHandle->data[0]), 0, sizeof(int) * (numFds + numInts)); > + tempHandle->data[0] = static_cast<int>(state.mOverlayId); > + mOverlayIdMap[current] = tempHandle; > + > + hwcLayer.handle = tempHandle; Hi Sushil, Actually, I still don't have any good idea about where I can put the overlayImage id. The extension field may be a choice, but it might be affected on the newer Android version. Do you have any suggestion? Thanks.
Comment on attachment 8502236 [details] [diff] [review] [WIP] Support OverlayImage in HWComposer2D (v2.3) Review of attachment 8502236 [details] [diff] [review]: ----------------------------------------------------------------- Since you have defined "HWC_OVERLAYIMAGE" flag, please pass it to HAL and keep all feature related customization in Vendor HAL. HwcComposer2D is a generic H/W Composer wrapper. Let's not customize it, it can easily break all other currently supported HWC use cases.
Attachment #8502236 - Flags: feedback?(sushilchauhan) → feedback-
(In reply to Sushil (On Vacation: 10/15 - 11/17) from comment #22) > Comment on attachment 8502236 [details] [diff] [review] > [WIP] Support OverlayImage in HWComposer2D (v2.3) > > Review of attachment 8502236 [details] [diff] [review]: > ----------------------------------------------------------------- > > Since you have defined "HWC_OVERLAYIMAGE" flag, please pass it to HAL and > keep all feature related customization in Vendor HAL. HwcComposer2D is a > generic H/W Composer wrapper. Let's not customize it, it can easily break > all other currently supported HWC use cases. Boris, I think you need to export HWC_OVERLAYIMAGE to each Vendor HAL[1], like bug 886415. Suggest to create another follow up bug. [1]https://www.codeaurora.org/cgit/quic/lf/b2g/build/tree/patch/ics_strawberry/hardware/qcom/display/remap-copybit-formats.patch?id=AU_LINUX_GECKO_B2G_ICS_1.2.01.02.00.019.011
Flags: needinfo?(boris.chiou)
(In reply to peter chang[:pchang][:peter] from comment #23) > (In reply to Sushil (On Vacation: 10/15 - 11/17) from comment #22) > > Comment on attachment 8502236 [details] [diff] [review] > > [WIP] Support OverlayImage in HWComposer2D (v2.3) > > > > Review of attachment 8502236 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Since you have defined "HWC_OVERLAYIMAGE" flag, please pass it to HAL and > > keep all feature related customization in Vendor HAL. HwcComposer2D is a > > generic H/W Composer wrapper. Let's not customize it, it can easily break > > all other currently supported HWC use cases. > > Boris, I think you need to export HWC_OVERLAYIMAGE to each Vendor HAL[1], > like bug 886415. Suggest to create another follow up bug. > > [1]https://www.codeaurora.org/cgit/quic/lf/b2g/build/tree/patch/ > ics_strawberry/hardware/qcom/display/remap-copybit-formats. > patch?id=AU_LINUX_GECKO_B2G_ICS_1.2.01.02.00.019.011 Sure, I file this follow bug, Bug 1086156, to pass HWC_OVERLAYIMAGE to each Vendor HAL.
Flags: needinfo?(boris.chiou)
Blocks: 1086156
Comment on attachment 8508551 [details] [diff] [review] [WIP] Support OverlayImage in HWComposer2D (v2.4) Review of attachment 8508551 [details] [diff] [review]: ----------------------------------------------------------------- The android version checking didn't cover all of your modification. Could we include the android version checking only when mOverlayImageSupport flag changes? ::: widget/gonk/HwcComposer2D.cpp @@ +413,5 @@ > + usingOverlayImage = true; > + } > +#endif > + > + if (!usingOverlayImage) { Why do we need to by-pass the following code for OverlayImage? @@ +518,5 @@ > + mOverlayIdMap[current] = tempHandle; > + > + hwcLayer.handle = tempHandle; > + hwcLayer.flags = HwcUtils::HWC_OVERLAYIMAGE; > + hwcLayer.compositionType = HWC_FRAMEBUFFER; I think you only need to modify the handle/flags, for other stuff, you don't need to modify them. @@ +752,5 @@ > + overlayImageComposite = true; > + break; > + } > + } > +#endif No need this code here, you can combine it with line 772. @@ +772,5 @@ > + if (mList->hwLayers[k].flags & HwcUtils::HWC_OVERLAYIMAGE) { > + // Handle OverlayImage case (partial composition) > + // Not only set overlayImage into the hwccomposer, but also > + // be composed by GPU (only borders) > + mHwcLayerMap[k]->SetLayerComposited(false); Suggest rename overlayImageComposite to TryGPUComposite. And set TryGPUComposite flag as true here. @@ +831,5 @@ > +} > + > +bool > +HwcComposer2D::TryHwCompositionforOverlayImage() > +{ Why do we need TryHwCompositionForOverlayImage when we already call TryHWComposition? ::: widget/gonk/HwcUtils.h @@ +46,5 @@ > // The flag will be set inside LayerRenderState > + HWC_FORMAT_RB_SWAP = 0x40, > + > + // Test > + HWC_OVERLAYIMAGE = 0x200 Please update the comment for this flag.
Comment on attachment 8508551 [details] [diff] [review] [WIP] Support OverlayImage in HWComposer2D (v2.4) Review of attachment 8508551 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for your feedback, Peter. I can remove the redundant Android version checking and only include it only when mOverlayImageSupport flag changes. ::: widget/gonk/HwcComposer2D.cpp @@ +413,5 @@ > + usingOverlayImage = true; > + } > +#endif > + > + if (!usingOverlayImage) { OverlayImage doesn't have gralloc buffer, so I think we shouldn't check these status and just by-pass these code. @@ +518,5 @@ > + mOverlayIdMap[current] = tempHandle; > + > + hwcLayer.handle = tempHandle; > + hwcLayer.flags = HwcUtils::HWC_OVERLAYIMAGE; > + hwcLayer.compositionType = HWC_FRAMEBUFFER; OK, I will move out this initialization. @@ +752,5 @@ > + overlayImageComposite = true; > + break; > + } > + } > +#endif I add this code because line 742 may set overlayComposite=false, so we will not arrive at line 772. Ex. if there is only one HWC_BLIT/HWC_FRAMEBUFFER layer and other layers are HWC_OVERLAY layers, we will by-pass line 758-820. (Actually I'm not sure what the real composition Types passed from hwcomposer are in HwcComposer2D.) @@ +772,5 @@ > + if (mList->hwLayers[k].flags & HwcUtils::HWC_OVERLAYIMAGE) { > + // Handle OverlayImage case (partial composition) > + // Not only set overlayImage into the hwccomposer, but also > + // be composed by GPU (only borders) > + mHwcLayerMap[k]->SetLayerComposited(false); Sure. @@ +831,5 @@ > +} > + > +bool > +HwcComposer2D::TryHwCompositionforOverlayImage() > +{ This is my original code, and I will remove this function in next patch. Thanks.
Attachment #8509296 - Flags: feedback?(pchang)
Attachment #8509301 - Flags: feedback?(pchang)
Attachment #8509301 - Flags: feedback?(sotaro.ikeda.g)
Attachment #8509301 - Flags: feedback?(sotaro.ikeda.g)
Comment on attachment 8511899 [details] [diff] [review] [WIP] Support OverlayImage in HWComposer2D (v5.1) Review of attachment 8511899 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gonk/HwcComposer2D.cpp @@ +927,5 @@ > + aLayer->displayFrame = {0, 0, mScreenRect.width, mScreenRect.height}; > +} > + > +bool > +HwcComposer2D::SetOverlayImageLayer(layers::Layer* aRoot, HwcLayer* aHwcLayer) s/aRoot/aLayer since you treat this a recursive call. But I have one question. Could we cache the HwcLayer for OverlayImage during prepare inside HwcComposer2D? Then we don't need to re-fill the HwcLayer again. ::: widget/gonk/HwcUtils.h @@ +45,5 @@ > // Swap the RB pixels of gralloc buffer, like RGBA<->BGRA or RGBX<->BGRX > // The flag will be set inside LayerRenderState > + HWC_FORMAT_RB_SWAP = 0x40, > + > + // OverlayImage Layer flag Please add more description about this flag
Attachment #8511899 - Flags: feedback?(pchang)
Attachment #8513220 - Flags: feedback?(pchang)
Attachment #8513220 - Flags: feedback?(chung)
1. Use mOverlayImageSupport to check if the hardware can handle our OverlayImage layers. 1. Check if this is an OverlayImage layer by mOverlayId (int32_t) of LayerRenderState. 2. OverlayImage should be set into the hwccomposer, and be composed by GPU (PS. GPU should draw the borders of this layer). a. If we already called Prepare(), everything looks good to me in HwcComposer2D::Render(). b. If we didn't called Prepare() because PrepareLayerList() was not finished, we should insert the OverlayImage layer into mList manually (, and we should traverse the layer tree again). c. The number of the Overlayimage layers might be more than one, so I use a vector to cache these layers and then copy them into mList. 3. Now we put the mOverlayId into reserved[0] (int32_t) (Need to be discussed) 4. In this patch, we use HWC_OVERLAYIMAGE to notify hwcomposer whether this layer is an overlayimage layer or not.
Attachment #8513220 - Attachment is obsolete: true
Comment on attachment 8513227 [details] [diff] [review] [WIP] Support OverlayImage in HWComposer2D (v5.4) Review of attachment 8513227 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but some detail may need discussion. ::: widget/gonk/HwcComposer2D.cpp @@ +497,5 @@ > + hwcLayer.handle = handle; > + > + hwcLayer.flags = 0; > + } > + Should add if (isOverlayImageLayer) { return false; } at the end of this function otherwise it can return true and make it prepare succeed while we need it fail. @@ +730,5 @@ > + // Note: OverlayImage layers still need to be composed by gpu, > + // so we should not set them composited. > + if (!(mList->hwLayers[k].flags | HwcUtils::HWC_OVERLAYIMAGE)) { > + mHwcLayerMap[k]->SetLayerComposited(true); > + } Maybe add gpuComposite = true; or return false; in case the mHWC->Prepare think it can handle HWC_OVERLAY (flag number collision). BTW, I think this is still dangerous. As we may not get overlay info before this function, and HWC HAL may use the HWC_OVERLAY(0x40) value and change it before here. @@ +949,5 @@ > + return false; > + } > + > + if (ContainerLayer* container = aLayer->AsContainerLayer()) { > + if (container->UseIntermediateSurface()) { The logic seems wrong to me here: ex. root / \ Container Overlay / Thebes(with mask) While traverse the tree here you will returns false very early while the Container should use intermediate surface.
Attachment #8513227 - Flags: feedback-
Comment on attachment 8513227 [details] [diff] [review] [WIP] Support OverlayImage in HWComposer2D (v5.4) Review of attachment 8513227 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gonk/HwcComposer2D.cpp @@ +414,4 @@ > } else { > + if (aLayer->AsColorLayer() && mColorFill) { > + fillColor = true; > + } else { I assume the mSurface is null and you can check the mOverlayId here. @@ +728,5 @@ > // Overlay Composition, set layer composition flag > // on mapped LayerComposite to skip GPU composition > + // Note: OverlayImage layers still need to be composed by gpu, > + // so we should not set them composited. > + if (!(mList->hwLayers[k].flags | HwcUtils::HWC_OVERLAYIMAGE)) { why u need to or them here? It should be "if (mList->hwLayers[k].flags & HwcUntils::HWC_OVERLAYIMAGE)". @@ +812,5 @@ > + for (int i = 0; i < length; ++i) { > + mList->hwLayers[i+1] = mCachedOverlayImageLayers[i]; > + ++mList->numHwLayers; > + } > + mCachedOverlayImageLayers.clear(); you still need to clear mCachedOverlayImageLayers if prepareOverlayImageLayer failed. ::: widget/gonk/HwcUtils.h @@ +49,5 @@ > + // OverlayImage Layer flag > + // If this layer is an OverlayImage layer, we should set this flag so that > + // our hwcomposer can handle it. The overlay ID and its layer rectangle > + // should also be set in the HwcLayer structure. > + HWC_OVERLAYIMAGE = 0x200 // Pass the attributes of OverlayImage to HAL If an OverlayImage // Layer exists and is able to handle by HAL, this flag will be // set during the PrepareLayerList. In the meantime, the id // and layer size of this OverlayImage will be set // inside hwc_layer_t struct
Comment on attachment 8513227 [details] [diff] [review] [WIP] Support OverlayImage in HWComposer2D (v5.4) Review of attachment 8513227 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gonk/HwcComposer2D.cpp @@ +414,4 @@ > } else { > + if (aLayer->AsColorLayer() && mColorFill) { > + fillColor = true; > + } else { Got it! @@ +497,5 @@ > + hwcLayer.handle = handle; > + > + hwcLayer.flags = 0; > + } > + Per our discussion today, we will not add the return here because we still need to traverse the layer tree even if the current layer is an OverlayImage Layer. Thanks. @@ +728,5 @@ > // Overlay Composition, set layer composition flag > // on mapped LayerComposite to skip GPU composition > + // Note: OverlayImage layers still need to be composed by gpu, > + // so we should not set them composited. > + if (!(mList->hwLayers[k].flags | HwcUtils::HWC_OVERLAYIMAGE)) { I will revise this part. Thanks. @@ +730,5 @@ > + // Note: OverlayImage layers still need to be composed by gpu, > + // so we should not set them composited. > + if (!(mList->hwLayers[k].flags | HwcUtils::HWC_OVERLAYIMAGE)) { > + mHwcLayerMap[k]->SetLayerComposited(true); > + } Ha, my bad, I should add gpuComposite = true here and add a warning message here. @@ +812,5 @@ > + for (int i = 0; i < length; ++i) { > + mList->hwLayers[i+1] = mCachedOverlayImageLayers[i]; > + ++mList->numHwLayers; > + } > + mCachedOverlayImageLayers.clear(); Actually, if prepareOverlayImageLayer failed, mCachedOverlayImageLayers should be empty. However, I should revise it to make it clearer. Thanks. @@ +949,5 @@ > + return false; > + } > + > + if (ContainerLayer* container = aLayer->AsContainerLayer()) { > + if (container->UseIntermediateSurface()) { Yes, you are right. This is a bug, and I should fix it. Thanks.
Comment on attachment 8513227 [details] [diff] [review] [WIP] Support OverlayImage in HWComposer2D (v5.4) Review of attachment 8513227 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gonk/HwcComposer2D.cpp @@ +949,5 @@ > + return false; > + } > + > + if (ContainerLayer* container = aLayer->AsContainerLayer()) { > + if (container->UseIntermediateSurface()) { Oops, sorry. I think we will continue to traverse this tree because its parent, "root", will continue to check another child, "Overlay". You can check Line 958. Thanks.
(In reply to Boris Chiou [:boris] from comment #41) > Comment on attachment 8513227 [details] [diff] [review] > [WIP] Support OverlayImage in HWComposer2D (v5.4) > > Review of attachment 8513227 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: widget/gonk/HwcComposer2D.cpp > @@ +949,5 @@ > > + return false; > > + } > > + > > + if (ContainerLayer* container = aLayer->AsContainerLayer()) { > > + if (container->UseIntermediateSurface()) { > > Oops, sorry. I think we will continue to traverse this tree because its > parent, "root", will continue to check another child, "Overlay". You can > check Line 958. Thanks. In my example, we should set UseIntermediateSurface in the container[1], and the container contains a Overlay, so it still bypass whole child traversing. [1]http://mxr.mozilla.org/mozilla-central/source/gfx/layers/Layers.cpp#1069
(In reply to Chiajung Hung [:chiajung] from comment #42) > (In reply to Boris Chiou [:boris] from comment #41) > > Comment on attachment 8513227 [details] [diff] [review] > > [WIP] Support OverlayImage in HWComposer2D (v5.4) > > > > Review of attachment 8513227 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: widget/gonk/HwcComposer2D.cpp > > @@ +949,5 @@ > > > + return false; > > > + } > > > + > > > + if (ContainerLayer* container = aLayer->AsContainerLayer()) { > > > + if (container->UseIntermediateSurface()) { > > > > Oops, sorry. I think we will continue to traverse this tree because its > > parent, "root", will continue to check another child, "Overlay". You can > > check Line 958. Thanks. > > In my example, we should set UseIntermediateSurface in the container[1], and > the container contains a Overlay, so it still bypass whole child traversing. > > [1]http://mxr.mozilla.org/mozilla-central/source/gfx/layers/Layers.cpp#1069 OK, it makes sense. I should remove this check. Thanks.
feature-b2g: 2.2? → ---
Whiteboard: [FT:Stream3] → [ft:conndevices]
Comment on attachment 8563285 [details] [diff] [review] [WIP] Support OverlayImage in HWComposer2D (v7) Review of attachment 8563285 [details] [diff] [review]: ----------------------------------------------------------------- Hi, Sotaro, As per our discussion in the meeting yesterday, this bug is trying to handle OverlayImage layers in HwcComposer2D. According to recent implementation of HwcComposer2D, I added some flags and layer tree traversal to find OverlayImage layers. (Actually, many codes look like hacks because it's not easy to add a special layer.) You can check my patch first or I can describe the mainly ideas in this patch to you. If it's possible, could you please give me some feedback? Thanks.
Attachment #8563285 - Flags: feedback?(sotaro.ikeda.g)
Comment on attachment 8563285 [details] [diff] [review] [WIP] Support OverlayImage in HWComposer2D (v7) Did feedback by offline.
Attachment #8563285 - Flags: feedback?(sotaro.ikeda.g)
Attached patch Support OverlayImage in HWComposer2D (v9) (obsolete) (deleted) — — Splinter Review
1. Use mOverlayImageSupport to check if the hardware can handle our overlayImage layers. 2. Check if this is an overlayImage layer by mOverlayId (int32_t) of LayerRenderState. 3. Remove early returns for Container layers in PrepareLayerList() to make sure we can find all overlayimage layers. 4. OverlayImage should be set into the hwccomposer, and be composed by GPU (PS. GPU should draw the borders of this layer). a. Full GPU: all layers are composed by GPU, so we should insert overlayimage layers before Prepare(). The number of the overlayimage layers might be more than one, so I use a vector to cache these layers and then copy them into mList. b. Partial hw composition: overlayimage layers are inserted into mList already, so everything is ok to us. c. It is impossible to use Full hw composition because overlayimage layers should be handled by GPU. 5. Now we put the mOverlayId into reserved[0] (int32_t) (Need to be discussed) 6. In this patch, we use HWC_OVERLAYIMAGE to notify hwcomposer whether this layer is an overlayimage layer or not. 7. The composition type of overlayImage Layers should be HWC_OVERLAY returned by the hwcomposer. If our hwcomposer gives us the wrong type, we will fix it in TryHWComposition() and force to gpuComposite.
Attachment #8569027 - Attachment is obsolete: true
(In reply to Boris Chiou [:boris] from comment #50) > Created attachment 8569030 [details] [diff] [review] > Support OverlayImage in HWComposer2D (v9) > > 1. Use mOverlayImageSupport to check if the hardware can handle > our overlayImage layers. > 2. Check if this is an overlayImage layer by mOverlayId (int32_t) of > LayerRenderState. > 3. Remove early returns for Container layers in PrepareLayerList() to > make sure we can find all overlayimage layers. > 4. OverlayImage should be set into the hwccomposer, and be composed by GPU > (PS. GPU should draw the borders of this layer). > a. Full GPU: all layers are composed by GPU, so we should insert > overlayimage layers before Prepare(). The number of the overlayimage > layers might be more than one, so I use a vector to cache these layers > and then copy them into mList. > b. Partial hw composition: overlayimage layers are inserted into > mList already, so everything is ok to us. > c. It is impossible to use Full hw composition because overlayimage > layers should be handled by GPU. > 5. Now we put the mOverlayId into reserved[0] (int32_t) (Need to be > discussed) > 6. In this patch, we use HWC_OVERLAYIMAGE to notify hwcomposer whether > this layer is an overlayimage layer or not. > 7. The composition type of overlayImage Layers should be HWC_OVERLAY > returned by the hwcomposer. If our hwcomposer gives us the wrong type, > we will fix it in TryHWComposition() and force to gpuComposite. try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6132132fc058
Comment on attachment 8569030 [details] [diff] [review] Support OverlayImage in HWComposer2D (v9) Review of attachment 8569030 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gonk/HwcComposer2D.cpp @@ +386,5 @@ > if (container->UseIntermediateSurface()) { > LOGD("Container layer needs intermediate surface"); > + ret = false; > + if (!mOverlayImageSupport) { > + return false; How about pass one more parameter 'traverseall' in the following API? Then we don't need to embedded mOverlayImageSupport inside preparelayerlist. HwcComposer2D::PrepareLayerList(Layer* aLayer, const nsIntRect& aClip, const Matrix& aParentTransform, bool traverseall = false) And call PrepareLayerList(alayer, aclip, aparentTransofrm, mOverlayImageSupport? true, false); @@ +416,5 @@ > + if (aLayer->AsColorLayer() && mColorFill) { > + fillColor = true; > + } else { > + LOGD("%s Layer doesn't have a gralloc buffer", aLayer->Name()); > + return false; If we had a layer without gralloc before imagelayer, the code here will return early. @@ +451,5 @@ > } > > // Buffer rotation is not to be confused with the angled rotation done by a transform matrix > // It's a fancy PaintedLayer feature used for scrolling > + if (state.BufferRotated() && !isOverlayImageLayer) { same problem as above. @@ +760,5 @@ > + // Note: OverlayImage layers still need to be composed by gpu, > + // so we should not set them composited. > + if (mList->hwLayers[k].flags & HwcUtils::HWC_OVERLAYIMAGE) { > + // still need gpu compose > + gpuComposite = true; No need to set gpuComposite as true here because line 786 will do, here only needs to bypass the SetLayerComposited call. @@ +840,5 @@ > } else { > mList->flags = HWC_GEOMETRY_CHANGED; > mList->numHwLayers = 2; > + > + SetAsSkipLayer(&mList->hwLayers[0]); The name of this function is not clear for me, and I don't see the benefit to change here. @@ +1017,5 @@ > MOZ_ASSERT(mHwcLayerMap.IsEmpty()); > + mOverlayImageLayerExist = false; > +#if ANDROID_VERSION >= 19 > + mCachedOverlayLayers.clear(); > +#endif Not consistent for overlayimage variables that some are under android 19 and others are not. ::: widget/gonk/HwcComposer2D.h @@ +122,5 @@ > int mMaxLayerCount; > bool mColorFill; > bool mRBSwapSupport; > + bool mOverlayImageSupport; > + bool mOverlayImageLayerExist; s/mOverlayImageLayerExist/mExistingOverlayImage
Comment on attachment 8569030 [details] [diff] [review] Support OverlayImage in HWComposer2D (v9) Review of attachment 8569030 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gonk/HwcComposer2D.cpp @@ +401,4 @@ > return false; > } > } > + return ret; Make sure all the children are checked, so we can retrieve all overlayimage layers from the layer tree. @@ +763,5 @@ > + // still need gpu compose > + gpuComposite = true; > + } else { > + mHwcLayerMap[k]->SetLayerComposited(true); > + } Assume an overlayImage layers is a kind of HWC_OVERLAY, but it still needs to be composed by GPU, so we don't SetLayerComposited(true) for overlayimage layers. @@ +847,5 @@ > + for (uint32_t i = 0; i < mCachedOverlayLayers.size(); ++i) { > + ++mList->numHwLayers; > + mList->hwLayers[i+1] = mCachedOverlayLayers[i]; > + } > +#endif This is a special case. All layers are composed by GPU, so there is no mHwc prepare. We should put the overlayimage layers into mList before Prepare().
Attachment #8569030 - Flags: feedback?(sotaro.ikeda.g)
Attachment #8569030 - Flags: feedback?(sotaro.ikeda.g)
Comment on attachment 8569030 [details] [diff] [review] Support OverlayImage in HWComposer2D (v9) Review of attachment 8569030 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gonk/HwcComposer2D.cpp @@ +386,5 @@ > if (container->UseIntermediateSurface()) { > LOGD("Container layer needs intermediate surface"); > + ret = false; > + if (!mOverlayImageSupport) { > + return false; It's a good idea. I can revise it in next patch. @@ +416,5 @@ > + if (aLayer->AsColorLayer() && mColorFill) { > + fillColor = true; > + } else { > + LOGD("%s Layer doesn't have a gralloc buffer", aLayer->Name()); > + return false; It's fine. Its parent(container layer) will continue to traverse next child until all children are checked. @@ +760,5 @@ > + // Note: OverlayImage layers still need to be composed by gpu, > + // so we should not set them composited. > + if (mList->hwLayers[k].flags & HwcUtils::HWC_OVERLAYIMAGE) { > + // still need gpu compose > + gpuComposite = true; Yes, it's a duplicate code. @@ +840,5 @@ > } else { > mList->flags = HWC_GEOMETRY_CHANGED; > mList->numHwLayers = 2; > + > + SetAsSkipLayer(&mList->hwLayers[0]); I will back it out. @@ +1017,5 @@ > MOZ_ASSERT(mHwcLayerMap.IsEmpty()); > + mOverlayImageLayerExist = false; > +#if ANDROID_VERSION >= 19 > + mCachedOverlayLayers.clear(); > +#endif OK, I will remove the version checker.
Blocks: 1026358
Attached patch Support OverlayImage in HWComposer2D (v10) (obsolete) (deleted) — — Splinter Review
1. Use mOverlayImageSupport to check if the hardware can handle our overlayImage layers. 2. Check if this is an overlayImage layer by mOverlayId (int32_t) of LayerRenderState. 3. Remove early returns for Container layers in PrepareLayerList() to make sure we can find all overlayimage layers. 4. OverlayImage should be set into the hwccomposer, and be composed by GPU (PS. GPU should draw the borders of this layer). a. Full GPU: all layers are composed by GPU, so we should insert overlayimage layers before Prepare(). The number of the overlayimage layers might be more than one, so I use a vector to cache these layers and then copy them into mList. b. Partial hw composition: overlayimage layers are inserted into mList already, so everything is ok to us. c. It is impossible to use Full hw composition because overlayimage layers should be handled by GPU. 5. Now we put the mOverlayId into reserved[0] (int32_t) (Need to be discussed) 6. In this patch, we use HWC_OVERLAYIMAGE to notify hwcomposer whether this layer is an overlayimage layer or not. 7. The composition type of overlayImage Layers should be HWC_OVERLAY returned by the hwcomposer. If our hwcomposer gives us the wrong type, we will fix it in TryHWComposition() and force to gpuComposite.
Attachment #8569030 - Attachment is obsolete: true
Hi all, Sorry to jump in but would like to share some info about gonk platform. Regarding to HWC module, the newest definition has one new compositionType called HWC_SIDEBAND. And this kind of layer need to carry one native handle id. It also mentioned, once HWC can't support this type this layer will be a solid color only because not supported by GPU. It seems a similar design with this bug now. [1] https://android.googlesource.com/platform/hardware/libhardware/+/master/include/hardware/hwcomposer.h
And there is a problem about the naming. 1. Refer to [1], Henri had mentioned the TV/HDMI layer we considered here is under the framebuffer layer so it might be a "underlay" not overlay. 2. In defintion of compositionType in HWC, there is already a type called HWC_OVERLAY. And the patch here adds a new flag called HWC_OVERLAYIMAGE. It seems developers can't understand the meaning we intend to do here. So there might be three options about this naming a. keep original one here. HWC_OVERLAYIMAGE b. change the name to something about UNDERLAY (of couse, we need to change OverlayImage class as well) c. Leverage what gonk platform added to L version. My personal preference would be b or c but not a. Any thought? [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1002823#c5
Flags: needinfo?(sotaro.ikeda.g)
Flags: needinfo?(pchang)
Flags: needinfo?(boris.chiou)
HWC_OVERLAY is just come from hardware pipe line name. Then I am not fun of using under as name.
Flags: needinfo?(sotaro.ikeda.g)
(In reply to Sotaro Ikeda [:sotaro] from comment #59) > HWC_OVERLAY is just come from hardware pipe line name. Then I am not fun of > using under as name. I think OVERLAY is not a hardware pipe line name. It would be an action showing to overlay something on top of framebuffertarget.
(In reply to Marco Chen [:mchen] from comment #58) > And there is a problem about the naming. > > 1. Refer to [1], Henri had mentioned the TV/HDMI layer we considered here is > under the framebuffer layer so it might be a "underlay" not overlay. > > 2. In defintion of compositionType in HWC, there is already a type called > HWC_OVERLAY. And the patch here adds a new flag called HWC_OVERLAYIMAGE. It > seems developers can't understand the meaning we intend to do here. > > So there might be three options about this naming > > a. keep original one here. HWC_OVERLAYIMAGE > b. change the name to something about UNDERLAY (of couse, we need to > change OverlayImage class as well) > c. Leverage what gonk platform added to L version. > > My personal preference would be b or c but not a. > Any thought? > > [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1002823#c5 Actually, HWC_OVRELAYIMAGE is a temporary name and we still can revise it. If anyone has good suggestions, I can upload a new patch to use after our discussion.
Flags: needinfo?(boris.chiou)
(In reply to Marco Chen [:mchen] from comment #60) > (In reply to Sotaro Ikeda [:sotaro] from comment #59) > > HWC_OVERLAY is just come from hardware pipe line name. Then I am not fun of > > using under as name. > > I think OVERLAY is not a hardware pipe line name. It would be an action > showing to overlay something on top of framebuffertarget. In the past, it might have such meaning. But I do not saw such meaning how its name is used in android framework.
(In reply to Marco Chen [:mchen] from comment #60) > (In reply to Sotaro Ikeda [:sotaro] from comment #59) > > HWC_OVERLAY is just come from hardware pipe line name. Then I am not fun of > > using under as name. > > I think OVERLAY is not a hardware pipe line name. It would be an action > showing to overlay something on top of framebuffertarget. Also in codeaurora HWC context, overlay just mention type of hardware. Overlay HWC or copybit HWC.
Historically, in android framework, Overlay class existed for similar usage to HWC_OVERLAY. Its role was for video rendering that is rendered without SurfaceFlinger composition. Since HC, the Overlay class was removed, all rendering go though SurfaceFlinger and hwc hal was added. Overlay was replaced by HWC_OVERLAY. Since Overlay era, the video frame that are rendered by using dedicated hardware was always rendered under framebuffer. But it is called Overlay since its first existence in android.
(In reply to Sotaro Ikeda [:sotaro] from comment #64) > > Since Overlay era, the video frame that are rendered by using dedicated > hardware was always rendered under framebuffer. But it is called Overlay > since its first existence in android. Otherwise, android can not draw video control ui/ camera ui over video image. The following is Overlay class that existed in the past. http://androidxref.com/2.1/xref/frameworks/base/libs/ui/Overlay.cpp http://androidxref.com/2.1/xref/frameworks/base/include/ui/Overlay.h
The following is a wikipedia's video overlay http://en.wikipedia.org/wiki/Video_overlay
Hi Sotaro, Thanks for spending time to give these information. I have no strong opinion here. Let's focus on land it in first. Sorry to have the diversion here.
No problem :-) Actually Overlay is used for very different things and it makes confusing.
Flags: needinfo?(howareyou322)
(In reply to Sotaro Ikeda [:sotaro] from comment #69) > By the way, android handle TV stream by HWC_SIDEBAND. Refer to comment 57, that is what I mentioned about AOSP for new HWC type - sideband against to overlay_image.
Hi Boris, how can we make this bug moving forward? I only saw the debate on the naming of HWC type from previous discussion.
Flags: needinfo?(boris.chiou)
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #71) > Hi Boris, how can we make this bug moving forward? I only saw the debate on > the naming of HWC type from previous discussion. Hi, According to previous discussion with sotaro, it's better to treat OverlayImage layers as a kind of HWC_OVERLAY and implement it together with partial HW composition. However, I only have a flame to test this use case and it doesn't use partial HW composition right now. We can continue this work after getting the development board (for TV products using OverlayImage layers) for testing. Thanks.
Flags: needinfo?(boris.chiou)
(In reply to Boris Chiou [:boris] from comment #72) > (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment > #71) > > Hi Boris, how can we make this bug moving forward? I only saw the debate on > > the naming of HWC type from previous discussion. > > Hi, > According to previous discussion with sotaro, it's better to treat > OverlayImage layers as a kind of HWC_OVERLAY and implement it together with > partial HW composition. However, I only have a flame to test this use case > and it doesn't use partial HW composition right now. We can continue this > work after getting the development board (for TV products using OverlayImage > layers) for testing. Thanks. If anyone wants to continue this bug, please feel free to take this. :)
(In reply to Boris Chiou [:boris] from comment #72) > (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment > #71) > > Hi Boris, how can we make this bug moving forward? I only saw the debate on > > the naming of HWC type from previous discussion. > > Hi, > According to previous discussion with sotaro, it's better to treat > OverlayImage layers as a kind of HWC_OVERLAY and implement it together with > partial HW composition. However, I only have a flame to test this use case > and it doesn't use partial HW composition right now. We can continue this > work after getting the development board (for TV products using OverlayImage > layers) for testing. Thanks. aries(Xperia z3c) has overlay hwc. Can see partial hwc composition on it.
Assignee: boris.chiou → nobody
Status: ASSIGNED → NEW
For tv stream, it seems better to use HWC_SIDEBAND like android.
I take the bug.
Assignee: nobody → sotaro.ikeda.g
(In reply to Sotaro Ikeda [:sotaro] from comment #76) > I take the bug. Thanks, Sotaro :)
No problem :)
Depends on: 1205713
Depends on: 1205725
Depends on: 1205773
Depends on: 1234472
Blocks: 1205773
No longer depends on: 1205773
Reused some code of attachment 8569701 [details] [diff] [review]. But a logic is a bit different.
Attachment #8569701 - Attachment is obsolete: true
Attachment #8704520 - Flags: feedback?(nical.bugzilla)
Attachment #8704520 - Flags: feedback?(nical.bugzilla)
Update a comment.
Attachment #8704520 - Attachment is obsolete: true
Attachment #8704521 - Flags: feedback?(nical.bugzilla)
Summary: [Stingray] Support OverlayImage in HWComposer2D → [Stingray] Support sideband stream in HWComposer2D
Component: Hardware Abstraction Layer (HAL) → Graphics: Layers
Comment on attachment 8704521 [details] [diff] [review] patch - Handle Sideband stream compositing in HwcComposer2D Review of attachment 8704521 [details] [diff] [review]: ----------------------------------------------------------------- I am not a HWC expert but from where I stand this looks good. ::: gfx/layers/Compositor.h @@ +455,5 @@ > virtual nsIWidget* GetWidget() const { return nullptr; } > > + virtual bool HasImageHostOverlays() { return false; } > + > + virtual void PutImageHostOverlay(ImageHostOverlay* aOverlay) {} nit: I'd rather call it AddImageHostOverlay, from a quick glance I find that "put" doesn't make it clear whether there can be several or only one overlay.
Attachment #8704521 - Flags: feedback?(nical.bugzilla) → feedback+
(In reply to Nicolas Silva [:nical] from comment #82) > > + virtual bool HasImageHostOverlays() { return false; } > > + > > + virtual void PutImageHostOverlay(ImageHostOverlay* aOverlay) {} > > nit: I'd rather call it AddImageHostOverlay, from a quick glance I find that > "put" doesn't make it clear whether there can be several or only one overlay. Thanks for the feedback. I'll update the patch.
Apply the feedback.
Attachment #8704521 - Attachment is obsolete: true
Attachment #8705029 - Flags: review?(nical.bugzilla)
Attachment #8705029 - Flags: review?(mwu)
Attachment #8705029 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8705029 [details] [diff] [review] patch - Handle Sideband stream compositing in HwcComposer2D Review of attachment 8705029 [details] [diff] [review]: ----------------------------------------------------------------- This mostly makes sense to me, though I'm not sure why we're allowed to put sideband layers on top of everything else when falling back to GPU rendering.. is that correct, or just the best wrong thing we can do?
Attachment #8705029 - Flags: review?(mwu) → review+
(In reply to Michael Wu [:mwu] from comment #85) > Comment on attachment 8705029 [details] [diff] [review] > patch - Handle Sideband stream compositing in HwcComposer2D > > Review of attachment 8705029 [details] [diff] [review]: > ----------------------------------------------------------------- > > This mostly makes sense to me, though I'm not sure why we're allowed to put > sideband layers on top of everything else when falling back to GPU > rendering.. is that correct, or just the best wrong thing we can do? It is just the best wrong thing that we can do. I am going to add a comment.
Update a comment. Carry "r=mwu, nical".
Attachment #8705029 - Attachment is obsolete: true
Attachment #8709246 - Flags: review+
No longer blocks: 1026358, 1086156
https://hg.mozilla.org/mozilla-central/rev/9672f0dc4b62
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: