Closed
Bug 877400
Opened 12 years ago
Closed 12 years ago
green video frame is drawn when there is no image to draw
Categories
(Firefox OS Graveyard :: General, defect)
Tracking
(blocking-b2g:leo+, firefox24 unaffected, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)
Tracking | Status | |
---|---|---|
firefox24 | --- | unaffected |
b2g18 | --- | fixed |
b2g18-v1.0.0 | --- | wontfix |
b2g18-v1.0.1 | --- | wontfix |
b2g-v1.1hd | --- | fixed |
People
(Reporter: sotaro, Assigned: sotaro)
References
Details
(Whiteboard: MiniWW)
Attachments
(1 file, 5 obsolete files)
+++ This bug was initially created as a clone of Bug #871485 +++
Created this bug from comment #38.
During Bug 871485, I often see green video frame drawn especially in following use case.
- Bug 871410 - Video playback error after refreshing a direct linked content.
The problem is caused by ShadowImageLayerOGL::RenderLayer(). It does not care about following situation.
- no graphic buffer for rendering. but try to render a texture.
It happened on rendering after VideoFrameContainer::ClearCurrentFrame().
Assignee | ||
Comment 1•12 years ago
|
||
There are two ways to solve the problem
-[1] skip the rendering
-[2] draw as black video frame
Assignee | ||
Updated•12 years ago
|
blocking-b2g: leo+ → ---
Assignee | ||
Updated•12 years ago
|
Severity: major → normal
Priority: P1 → --
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → sotaro.ikeda.g
Assignee | ||
Comment 2•12 years ago
|
||
Nominate to leo, because green video frame gives very bad impression to users.
blocking-b2g: --- → leo?
Assignee | ||
Comment 3•12 years ago
|
||
Bug 876838 might happen by this defect.
Assignee | ||
Comment 4•12 years ago
|
||
Assignee | ||
Comment 5•12 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #4)
> Created attachment 755629 [details] [diff] [review]
> patch - skip rendering if there is no image to draw
By applying the patch, I confirmed the problem is fixed on v1.1 buri with Bug 871485's patches.
Assignee | ||
Comment 6•12 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #1)
> There are two ways to solve the problem
> -[1] skip the rendering
> -[2] draw as black video frame
roc, which way is the correct fix in this case? Can I have a comment about it? attachment 755629 [details] [diff] [review] choosed [1] for simplicity.
Flags: needinfo?(roc)
Can we use the previous frame? That would be best.
Otherwise I think black would be best. Showing what's underneath the video would be confusing since that's never normally visible --- there could be strange Web content hidden under there.
Flags: needinfo?(roc)
Assignee | ||
Comment 8•12 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> Can we use the previous frame? That would be best.
>
> Otherwise I think black would be best. Showing what's underneath the video
> would be confusing since that's never normally visible --- there could be
> strange Web content hidden under there.
Thanks for the comment. In this case, previous frame can not be used. It was already send back to content process and destroyed. Then I am going to implement [2](draw black).
Assignee | ||
Updated•12 years ago
|
Summary: green video frame is drawin when there is no image to draw → green video frame is drawn when there is no image to draw
Assignee | ||
Comment 9•12 years ago
|
||
Created patch by reusing the code of RenderColorLayer(). Confirmed the patch works on v1.1 buri.
Attachment #755629 -
Attachment is obsolete: true
Assignee | ||
Comment 10•12 years ago
|
||
Previous patch was wrong one. Replace it.
Attachment #755691 -
Attachment is obsolete: true
Updated•12 years ago
|
blocking-b2g: leo? → leo+
Whiteboard: miniWW
Assignee | ||
Comment 11•12 years ago
|
||
Comment on attachment 755693 [details] [diff] [review]
patch - draw black when there is no image to draw
roc, can you review the path?
Attachment #755693 -
Flags: review?(roc)
Comment on attachment 755693 [details] [diff] [review]
patch - draw black when there is no image to draw
Review of attachment 755693 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/opengl/ImageLayerOGL.cpp
@@ +1018,5 @@
> } while (mTexImage->NextTile());
> }
> #ifdef MOZ_WIDGET_GONK
> + } else if (mSize == gfxIntSize(0, 0)) {
> + // when there is no image for rendering, draw the region by black color
"fill the region with black"
@@ +1035,5 @@
> + float opacity = GetEffectiveOpacity() * color.a;
> + color.r *= opacity;
> + color.g *= opacity;
> + color.b *= opacity;
> + color.a = opacity;
why are we doing all this? just do
gfxRGBA color(0.0, 0.0, 0.0, GetEffectiveOpacity());
Attachment #755693 -
Flags: review?(roc) → review+
Blocks: 877032
Assignee | ||
Comment 13•12 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> Comment on attachment 755693 [details] [diff] [review]
> patch - draw black when there is no image to draw
>
> Review of attachment 755693 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: gfx/layers/opengl/ImageLayerOGL.cpp
> @@ +1018,5 @@
> > } while (mTexImage->NextTile());
> > }
> > #ifdef MOZ_WIDGET_GONK
> > + } else if (mSize == gfxIntSize(0, 0)) {
> > + // when there is no image for rendering, draw the region by black color
>
> "fill the region with black"
Update in next patch.
>
> @@ +1035,5 @@
> > + float opacity = GetEffectiveOpacity() * color.a;
> > + color.r *= opacity;
> > + color.g *= opacity;
> > + color.b *= opacity;
> > + color.a = opacity;
>
> why are we doing all this? just do
> gfxRGBA color(0.0, 0.0, 0.0, GetEffectiveOpacity());
Yes, I'll update in next patch. Thanks.
Assignee | ||
Comment 14•12 years ago
|
||
Carry "roc: review+". This patch could be applyed only for b2g18.
master's source code do not have a same function by gfx layer refactoring.
Attachment #755693 -
Attachment is obsolete: true
Attachment #755743 -
Flags: review+
Updated•12 years ago
|
Whiteboard: miniWW → MiniWW
Target Milestone: --- → 1.1 QE2 (6jun)
Assignee | ||
Comment 15•12 years ago
|
||
fix "else if" to following.
+ } else if (mExternalBufferTexture.IsAllocated() && mSize == gfxIntSize(0, 0)) {
Attachment #755743 -
Attachment is obsolete: true
Assignee | ||
Comment 16•12 years ago
|
||
Comment on attachment 755795 [details] [diff] [review]
patch v3 for b2g18 - draw black when there is no image to draw
roc, can you review again? This patch changes only "else if" line. During rechecking the patch. I thought this change is necessary.
Attachment #755795 -
Flags: review?(roc)
Attachment #755795 -
Flags: review?(roc) → review+
Assignee | ||
Comment 17•12 years ago
|
||
Assignee | ||
Comment 18•12 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #17)
> https://tbpl.mozilla.org/?tree=Try&rev=35744f50839b
reftest failed a log. I am not sure it is related to the change.
Assignee | ||
Comment 19•12 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #18)
> (In reply to Sotaro Ikeda [:sotaro] from comment #17)
> > https://tbpl.mozilla.org/?tree=Try&rev=35744f50839b
>
> reftest failed a log. I am not sure it is related to the change.
The change should affect only to video playback with hw video codec and camera playback.
Assignee | ||
Comment 20•12 years ago
|
||
Following part might be wrong.
} else {
mSize = gfxIntSize(0, 0);
mPictureRect = nsIntRect(0, 0, 0, 0);
}
Assignee | ||
Comment 21•12 years ago
|
||
I pushed different patch to try that restricted to affect only to hw video codec rendering and camera preview. but reftest still failed. It seems that base source code has problem.
https://tbpl.mozilla.org/?tree=Try&rev=765c22449661
Assignee | ||
Comment 22•12 years ago
|
||
I pushed the no change patch to tryserver and got same result. So the patch make no problem. It seems that some tests do not run correctly on b2g18
https://tbpl.mozilla.org/?tree=Try&rev=9d5b91fd1859
Assignee | ||
Comment 23•12 years ago
|
||
Add header to the patch. Carry "roc: review+".
Attachment #755795 -
Attachment is obsolete: true
Assignee | ||
Comment 24•12 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #23)
> Created attachment 756675 [details] [diff] [review]
> patch v4 for b2g18 - draw black when there is no image to draw
>
> Add header to the patch. Carry "roc: review+".
The patch is only for b2g18. There is no same function on master.
Assignee | ||
Updated•12 years ago
|
status-b2g18:
--- → affected
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 25•12 years ago
|
||
Comment 26•12 years ago
|
||
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → wontfix
status-b2g-v1.1hd:
--- → fixed
status-firefox24:
--- → unaffected
Updated•12 years ago
|
Flags: in-moztrap-
You need to log in
before you can comment on or make changes to this bug.
Description
•