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

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[gbennett]

Attachment: GalleryEditEffectsColorLog.txt

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.

Comment 1

[Hema Koka [:hema]]

Diego,

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

Thanks
Hema

Comment 2

[Sotaro Ikeda [:sotaro]]

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

Comment 3

[Sotaro Ikeda [:sotaro]]

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

Comment 4

[Diego Wilson [:diego]]

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?

Comment 5

[Diego Marcos [:dmarcos]]

:sotaro. I don't have the context to determine if the patch solves the problem properly. Do you want to take this bug?

Comment 6

[Diego Wilson [:diego]]

@bjacob are you familiar with the code in comment 2? It looks like a workaround more than a proper fix.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 8

[Hema Koka [:hema]]

Milan, need help from your team for this bug fix.

Thanks
Hema

Comment 9

[Diego Wilson [:diego]]

@vladv do you know why readpixels was used in the code in comment 2? Would you recommend turning this on by default?

Comment 10

[Sotaro Ikeda [:sotaro]]

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

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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?

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

Parking with Vlad while the conversation is going on.  The rest, please don't disappear :)

Comment 14

[Diego Wilson [:diego]]

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.

Comment 15

[Sotaro Ikeda [:sotaro]]

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.

Comment 16

[Sotaro Ikeda [:sotaro]]

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

Comment 17

[Sotaro Ikeda [:sotaro]]

FPS of [1] with OpenGL composition was very low. It seems caused by temporary gralloc contention between WebGL rendering and OpenGL composition.

Comment 18

[Sotaro Ikeda [:sotaro]]

[3] might have a good effect also to Bug 915001 Comment 15's problem.

Comment 19

[Sotaro Ikeda [:sotaro]]

attachment 819267 [details] [diff] [review] in Bug 767484 is [3]'s wip patch. The patch do not work correctly on SkiaGL backed Canvas.

Comment 20

[Sotaro Ikeda [:sotaro]]

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.

Comment 21

[Diego Wilson [:diego]]

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

Comment 22

[Sotaro Ikeda [:sotaro]]

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.

Comment 23

[Sotaro Ikeda [:sotaro]]

Attachment: patch - enable fence with readpixels

Comment 24

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=9d7f1d642295

Comment 25

[Sotaro Ikeda [:sotaro]]

Comment on attachment 820596 [details] [diff] [review]
patch - enable fence with readpixels

vlad, can you review the patch?

Comment 26

[Lucas Adamski [:ladamski]]

Looks like we're close, but this is blocking partner certification.

Comment 28

[Sotaro Ikeda [:sotaro]]

Comment on attachment 820596 [details] [diff] [review]
patch - enable fence with readpixels

I got r+ from vlad orally.

Comment 29

[Sotaro Ikeda [:sotaro]]

Attachment: patch v2 - Enable fence with readpixels

Committable patch. Carry 'vlad=r+'.

Comment 30

[Sotaro Ikeda [:sotaro]]

Attachment: patch v3 - Enable fence with readpixels

unbitrot. Carry 'vlad=r+'.

Comment 31

[Sotaro Ikeda [:sotaro]]

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.

Comment 32

[Ed Morley [:emorley]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c33577a1cbe6

Comment 33

[Diego Wilson [:diego]]

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

Comment 34

[Sotaro Ikeda [:sotaro]]

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

Comment 35

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c33577a1cbe6

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/32bc5b2be9dd

Comment 37

[Sotaro Ikeda [:sotaro]]

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