Closed Bug 917444 Opened 6 years ago Closed 6 years ago

[B2G][Gallery][Edit]Effects only filter part of an image and sometimes both parts incorrectly

Categories

(Core :: Graphics, defect)

26 Branch
ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
1.3 Sprint 3 - 10/25
blocking-b2g koi+
Tracking Status
firefox25 --- wontfix
firefox26 --- fixed
firefox27 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: gbennett, Assigned: sotaro)

References

()

Details

(Keywords: regression, Whiteboard: burirun1, burirun2)

Attachments

(2 files, 2 obsolete files)

Description:
Selecting any of the Effects in Gallery > Edit cause only part of the image to be filtered and usually both halves incorrectly. The color/saturation/contrast can also change if you press the same filter repeatedly, none of which is a correct representation of the filter or the photo.

Repro Steps:
1) Updated Buri 1.2 mozRIL to Build ID: 20130916040205
2) Have a few images in the Gallery
3) Go to the Gallery and click on an image
4) Select Edit (left center icon)
5) Select Effects (right center icon)
6) Select through all of the effects

Actual:
Effect filters do not match the filter preview thumbnails at the bottom of the screen and only partially change the image.

Expected:
Effect filters correctly match the preview thumbnails and when an effect is added it changes the image fully.

Environmental Variables
Devices: Buri 1.2 mozRIL
Build ID: 20130916040205
Gecko: http://hg.mozilla.org/mozilla-central/rev/c4bcef90cef9
Gaia: a0079597d510ce8ea0b9cbb02c506030510b9eeb
Platform Version: 26.0a1

Notes: So it is highly likely that even if the partial filter issue was fixed, this still wouldn't be fully fixed till 899257 is resolved as right when you launch the edit section the image color/stauration/contrast is incorrect and thus makes further effect filtering even more incorrect.
Repro frequency: 100%, 12/12
See attached: http://www.youtube.com/watch?v=ApHp-cXXKsI, GalleryEditEffectsColorLog.txt

Regression Window:

Does not repro on Leo 1.1

Oldest Reproducible build:

Environmental Variables
Device: Buri 1.2 mozRIL
Build ID: 20130621031231
Gecko: http://hg.mozilla.org/mozilla-central/rev/7ba8c86f1a56
Gaia: e2f19420fa6a26c4287588701efaec09a750dba1
Platform Version: 24.0a1

We don't have an older Buri master build.
Assignee: nobody → dmarcos
Keywords: regression
blocking-b2g: --- → koi?
QA Contact: mozillamarcia.knous
Diego,

Please take a look at this and update with your findings.

Thanks
Hema
QA Contact: mozillamarcia.knous
blocking-b2g: koi? → koi+
If "gfx.gralloc.fence-with-readpixels" is true, the following patch for Fence() is used. By enabling it, it seems the partial drawing problem seems to be fixed.

void
SharedSurface_Gralloc::Fence()
{
    // We should be able to rely on genlock write locks/read locks.
    // But they're broken on some configs, and even a glFinish doesn't
    // work.  glReadPixels seems to, though.
    if (sForceReadPixelsToFence) {
        mGL->MakeCurrent();
        // read a 1x1 pixel
        unsigned char pixels[4];
        mGL->fReadPixels(0, 0, 1, 1, LOCAL_GL_RGBA, LOCAL_GL_UNSIGNED_BYTE, &pixels[0]);
    }
}

http://mxr.mozilla.org/mozilla-central/source/gfx/gl/SharedSurfaceGralloc.cpp#144
:diego, do you know how we should fence the buffer on v1.2? In the code in Comment 2 does the fence by read a 1*1 pixcel. It is not an ideal way of the fence.
Flags: needinfo?(dwilson)
Whiteboard: burirun1 → burirun1, burirun2
I'm able to reproduce this on v1.2 and indeed the patch in comment 2 fixes it.

AFAIK if this is a gralloc backed buffer and all reads and writes are done through GL then genlock() should take care of it. Are my assumptions correct?
Flags: needinfo?(dwilson)
:sotaro. I don't have the context to determine if the patch solves the problem properly. Do you want to take this bug?
Blocks: 916996
@bjacob are you familiar with the code in comment 2? It looks like a workaround more than a proper fix.
Flags: needinfo?(bjacob)
Yes, it is a work-around for the absence of proper fence support in the OpenGL driver on whichever driver this was tested on when that code was written. I didn't write or review this code, which was added in bug 843599. Its author is :vladv, and reviewers are :jrmuizel, :nical and :jgilbert. I suggest asking them -- I really amn't very competent here.
Flags: needinfo?(bjacob)
Milan, need help from your team for this bug fix.

Thanks
Hema
Assignee: dmarcos → milan
@vladv do you know why readpixels was used in the code in comment 2? Would you recommend turning this on by default?
Flags: needinfo?(vladimir)
I saw almost same code in the following.
  EGLBoolean eglSwapBuffers(EGLDisplay dpy, EGLSurface draw)
  http://androidxref.com/4.3_r2.1/xref/frameworks/native/opengl/libs/EGL/eglApi.cpp#993
It's effectively a glFinish(), but a "for reals" one, because glFinish doesn't work in some cases.  We don't have explicit synchronization primitives, and from my previous discussions I thought we could rely on genlock doing the right thing under the hood for us.  But some partners disabled genlock, so we added this as an optional workaround.  It can impact performance.  Does the platform you're testing on have genlock enabled and working?
Flags: needinfo?(vladimir)
Duplicate of this bug: 901225
Parking with Vlad while the conversation is going on.  The rest, please don't disappear :)
Assignee: milan → vladimir
I don't recommend disabling genlock but I understand that it may be unavoidable on certain devices. If buri is one of these devices then it sounds like the "gfx.gralloc.fence-with-readpixels" pref should be set on this device.
We have 3 choices now.
[1] Do nothing about frame complete(current behavior)
[2] Ensure frame complete by reading one pixcel
[3] ANativeWindow backed EGLSurface

[1] causes incorrect drawing now, we can not use [1] as a product. But [2] decrease the fps than [1]. So, I am going to think about [3]. [3] was already investigated a little bit in the past in Bug 767484.
The following is a fps of crystalskull on master hamachi. Compare the fps of HwComposer composition and OpenGL composition. [3] got good fps on the both cases.
[1]
  - HwComposer : 32-33fps
  - OpenGL: 0fps(< 1 fps), a lot of frame drops?
[2]
  - HwComposer : 30-33fps
  - OpenGL:25-26fps
[3]
  - HwComposer : 32-33fps
  - OpenGL : 32-33fps
FPS of [1] with OpenGL composition was very low. It seems caused by temporary gralloc contention between WebGL rendering and OpenGL composition.
[3] might have a good effect also to Bug 915001 Comment 15's problem.
attachment 819267 [details] [diff] [review] in Bug 767484 is [3]'s wip patch. The patch do not work correctly on SkiaGL backed Canvas.
For b2g v1.2, it seems better to choose [2]. [1] causes Bug 917444 and Bug 894847. From Comment 16, [2]'s crystalskull fps of HwComposer case is almost same as [1]'s one and [2]'s fps of OpenGL composition case is much better than [1]. [3] could have better fps than [2], but correct implementation of [3] becomes big change. It is better to implement it in v1.3.
(In reply to Sotaro Ikeda [:sotaro] from comment #15)
> [1] causes incorrect drawing now, we can not use [1] as a product.

If I understand correctly this is not an issue on commercial products because genlock works correctly on those devices, right?
By the discussion in graphics work week in Paris.
- For the short term, fix the problem by [2].
- For the longer term, implement [3]. maybe b2g v1.3.
Attachment #820596 - Attachment description: enable fence with readpixels → patch - enable fence with readpixels
Comment on attachment 820596 [details] [diff] [review]
patch - enable fence with readpixels

vlad, can you review the patch?
Attachment #820596 - Flags: review?(vladimir)
Target Milestone: --- → 1.2 C3(Oct25)
Looks like we're close, but this is blocking partner certification.
Duplicate of this bug: 929886
Blocks: GFXB2G1.2
Component: Gaia::Gallery → Graphics
Product: Boot2Gecko → Core
Version: unspecified → 26 Branch
Comment on attachment 820596 [details] [diff] [review]
patch - enable fence with readpixels

I got r+ from vlad orally.
Attachment #820596 - Flags: review?(vladimir) → review+
Assignee: vladimir → sotaro.ikeda.g
Committable patch. Carry 'vlad=r+'.
Attachment #820596 - Attachment is obsolete: true
unbitrot. Carry 'vlad=r+'.
Attachment #821659 - Attachment is obsolete: true
Attachment #821664 - Flags: review+
Keywords: checkin-needed
I have heard that force syncing by reading one pixel does not work on some GPUs like PowerVR. It causes poor performance in the GPU. Current b2g devices does not use PowerVR. So, it is not a problem in current b2g devices. But the current way of syncing should be replaced by Bug 767484 soon.
(In reply to Sotaro Ikeda [:sotaro] from comment #30)
> Created attachment 821664 [details] [diff] [review]
> patch v3 - Enable fence with readpixels
> 
> unbitrot. Carry 'vlad=r+'.

Did this really needed to be enabled for all devices?
Flags: needinfo?(gbennett)
Flags: needinfo?(gbennett) → needinfo?(sotaro.ikeda.g)
(In reply to Diego Wilson [:diego] from comment #33)
> > unbitrot. Carry 'vlad=r+'.
> 
> Did this really needed to be enabled for all devices?

It seems necessary for all adreno 200 devices. Anyway this is a temporary solution not to block developement. Actual solution of Bug 767484's target is changed to koi.
Flags: needinfo?(sotaro.ikeda.g)
https://hg.mozilla.org/mozilla-central/rev/c33577a1cbe6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: 1.2 C3(Oct25) → 1.3 Sprint 3 - 10/25
> > Did this really needed to be enabled for all devices?
> 
> It seems necessary for all adreno 200 devices. Anyway this is a temporary
> solution not to block developement. Actual solution of Bug 767484's target
> is changed to koi.

By the discussion in graphic team, Bug 767484's target is changed to 1.3 again.
Blocks: 894847
You need to log in before you can comment on or make changes to this bug.