1071514 - [Woodduck][Messages] The picture will flicker once when you take one picture as attachment

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Julien Wajsberg [:julienw] (PTO -> may 13)]

STR:
1.Launch message
2.Add one new message
3.Tap attachment button
4.Tap"Camera"to take one picture

[Expected Result]

The picture should not flicker 

[Error]

The picture will flicker once

Comment 1

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Steve in bug 1068490:


It's because the image return from camera exceed the mms size and I assume the flicker effect is we are resizing the image and replace it with the smaller image at this moment.

Hi Julien, I have a idea in my mind that we actually don't need to update the entire attachment node in composer, we could just update the attachments map and simply update the image size text in the banner instead of rendering another node and replace it, wdyt?

Comment 2

[Julien Wajsberg [:julienw] (PTO -> may 13)]

(In reply to Julien Wajsberg [:julienw] from comment #1)
> Steve in bug 1068490:
> 
> 
> It's because the image return from camera exceed the mms size and I assume
> the flicker effect is we are resizing the image and replace it with the
> smaller image at this moment.
> 
> Hi Julien, I have a idea in my mind that we actually don't need to update
> the entire attachment node in composer, we could just update the attachments
> map and simply update the image size text in the banner instead of rendering
> another node and replace it, wdyt?

Here is another proposal: let's not add the image while it's resizing. Instead, let's add the generic image for <image> attachments, and add the resized attachment on top of it when it's ready.

Why am I proposing this? I'm afraid that keeping the unresized image blob takes too much memory. What happens if the user adds other images then?

On the other side, if the unresized image blob is a file-backed blob, then it doesn't take that much memory, and probably less than the resized mem-based blob (that we still need anyway). So maybe we can do what you're proposing for file-based blob, but what I propose for memory-based blob (eg: Cropped images). What do you think?

Maybe you can discuss with Jenny about this too, to know if it's ok to have 2 different behaviors depending on the blob.

Comment 3

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #2)
> Here is another proposal: let's not add the image while it's resizing.
> Instead, let's add the generic image for <image> attachments, and add the
> resized attachment on top of it when it's ready.
> 
> Why am I proposing this? I'm afraid that keeping the unresized image blob
> takes too much memory. What happens if the user adds other images then?
> 
Since ImageUtils patch landed in master and seems not necessary to uplift to other branch, the blob size won't affect thumbnail generation process now. The image in attachment node is the thumbnail of the original blob, and it should be downsampled by moz-samplesize. I don't think it's a serious memory impact that using the thumbnail of original/resize blob. The only difference is when image is way too big that even the downsampled thumbnail resolution is much bigger than thumbnail container(but we ignore the image size over 1.5mb). It seems not a common case here, so it's the reason why I propose to keep the original thumbnail. Or you still think that platform will keep the fullsize memory for downsampled image?

> On the other side, if the unresized image blob is a file-backed blob, then
> it doesn't take that much memory, and probably less than the resized
> mem-based blob (that we still need anyway). So maybe we can do what you're
> proposing for file-based blob, but what I propose for memory-based blob (eg:
> Cropped images). What do you think?
> 
> Maybe you can discuss with Jenny about this too, to know if it's ok to have
> 2 different behaviors depending on the blob.

Just ask Jenny, but she thought even the placeholder -> thumbnail changes is also kind of flicker to user. Keeping the original thumbnail without change is her preferable choice since it takes less response time for user, but if the memory consumption is really a problem, she would prefer not to render any image while resizing.

Comment 4

[Julien Wajsberg [:julienw] (PTO -> may 13)]

(In reply to Steve Chung [:steveck] from comment #3)
> (In reply to Julien Wajsberg [:julienw] from comment #2)
> > Here is another proposal: let's not add the image while it's resizing.
> > Instead, let's add the generic image for <image> attachments, and add the
> > resized attachment on top of it when it's ready.
> > 
> > Why am I proposing this? I'm afraid that keeping the unresized image blob
> > takes too much memory. What happens if the user adds other images then?
> > 
> Since ImageUtils patch landed in master and seems not necessary to uplift to
> other branch, the blob size won't affect thumbnail generation process now.
> The image in attachment node is the thumbnail of the original blob, and it
> should be downsampled by moz-samplesize. I don't think it's a serious memory
> impact that using the thumbnail of original/resize blob. The only difference
> is when image is way too big that even the downsampled thumbnail resolution
> is much bigger than thumbnail container(but we ignore the image size over
> 1.5mb). It seems not a common case here, so it's the reason why I propose to
> keep the original thumbnail. Or you still think that platform will keep the
> fullsize memory for downsampled image?

I'm talking about the blob itself, I mean the undecoded image data. I agree with you for the decoded image data, even if I'm sure we can find big images that are small in size, but I'm not concerned for this.

> (but we ignore the image size over 1.5mb)

Should we revisit this limit now that we use mozsamplesize, by the way?

> 
> > On the other side, if the unresized image blob is a file-backed blob, then
> > it doesn't take that much memory, and probably less than the resized
> > mem-based blob (that we still need anyway). So maybe we can do what you're
> > proposing for file-based blob, but what I propose for memory-based blob (eg:
> > Cropped images). What do you think?
> > 
> > Maybe you can discuss with Jenny about this too, to know if it's ok to have
> > 2 different behaviors depending on the blob.
> 
> Just ask Jenny, but she thought even the placeholder -> thumbnail changes is
> also kind of flicker to user.

Let me summarize the issue: here we see a flicker because while we render the resized thumbnail image we have a white square replacing the full-size image. But the image is always rendered on top of the placeholder if I'm not wrong, that's why I was proposing this. I think that if we don't replace any displayed image by a white area before displaying something else, then we don't have a flickering.

Now I wonder if it's possible to display in the DOM the resized image only once it's rendered (maybe not: maybe gecko does not want to even start rendering until it's displayed).

> Keeping the original thumbnail without change
> is her preferable choice since it takes less response time for user, but if
> the memory consumption is really a problem, she would prefer not to render
> any image while resizing.

I'd like to insist here: I'm sure displaying a placeholder while the image is resized will look better. Maybe we can try both (I think the code difference will be tiny) and decide.

Another possibility is to display the placeholder only if the resize operation takes too much time (say more than 200ms). But I'm sure that it's better than adding nothing or a adding a white square.

Also, I know we have a banner on the top but it's not where the user is looking at this moment. Maybe we should move this information inside the message box, on top of the placeholder. This would be for a separate bug, but I think this would give the right information at the right moment in the right place.

NI Jenny :)

Comment 5

[Jenny Lee]

Hello Julien,

I think our top priority is to avoid the flicker that gives user a broken viewing experience. If it's possible, I'd love to see how the place holder solution works before making any decision on this. (Any solution that serves the goal would be good enough for me!)

About repositioning the banner in message..Since we use banner for quite a few different purposes in message, it'd be better if we keep the positioning consistent. Eventhough the upper area is not be the visual focus when user is attaching photo, user can surely notice the appearance of the banner thanks to the strong peripheral vision :) So, I'm inclined to keep the current behavior. Thanks!

Comment 7

[Luke Chang [:lchang] (inactive)]

This is my trial patch which may avoid flickering when replacing the attachments.
https://github.com/luke-chang/gaia/commit/6136a06bc8a5adfa5a97ecd60952d8f8649945b6

However, it uses 3000ms as a magic number because I have no idea yet how we can get the "ready/loaded" event of the new element. I'll keep trying next week if possible.

Comment 8

[Julien Wajsberg [:julienw] (PTO -> may 13)]

I started something a little more involved, about how we append the resized image. So I'm taking it. However maybe we'll need something more simple for v2.0 and in that case your patch can be useful. But we'll see.

Comment 9

[Julien Wajsberg [:julienw] (PTO -> may 13)]

WIP is https://github.com/julienw/gaia/compare/1071514-flickr-attachment, it's mostly an initial refactoring.

Comment 10

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Attachment: github PR

Unit tests are not ready yet.

This is what you proposed earlier. On one side, it's a quite simple code. On the other side, I don't like that we keep the big undecoded blob in memory. So I'll continue working on the other patch.

But maybe this patch is good enough to go to v2.0 and v2.1.

Comment 11

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Attachment: github PR - more invasive

Hey Steve,

in this PR, I change a lot how the resizing images operation is called. Please tell me what you think.

This is a lot more invasive than I expected, so I think this should land only on master if you think the approach makes sense.

There are several ideas here:

* displaying the placeholder behind the image; therefore, we see the placeholder while the image is loaded. I'm not sure we should do it for the images that don't need to be resized (although it could still need to be resized if we add more images then)
* it's possible to update the thumbnail without renerating the whole iframe (same idea than in the first patch)
* in compose.js, onContentChanged no longer handles resizing image. I find this a lot more easy to understand.
* in compose.js, fromDraft and fromMessage both benefit from the batch mechanism we introduced when sharing several images.

The CSS is not perfect yet; I think the background image placeholder is misplaced by some pixels.

Unit tests are not ready.

We should consider not showing the "message exceeds the maximum size" banner while we resize. This can be made in a separate bug, what do you think?

Comment 12

[Steve Chung [:steveck]]

I think the 1st patch is good enough since the main idea is thumbnail update without generating another iframe. The only concern if the heigh of iframe is not fixed while in preview mode, and you can see the iframe resized when image loaded/rendered. Since the image attachment size is fixed currently, maybe we could also set the iframe with fixed height to prevent unnecessary iframe resizing.

Sorry I haven't looked into your 2nd patch yet, but Jenny got some different thought about your image place holder idea after she saw your patch actually running on device, so ni? her here and I'll left more thought for both patch, thanks!

Comment 13

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #11)
> 
> We should consider not showing the "message exceeds the maximum size" banner
> while we resize. This can be made in a separate bug, what do you think?

About this issue, yes, it would be better without this displaying while resizing the image.

Comment 14

[Julien Wajsberg [:julienw] (PTO -> may 13)]

(In reply to Steve Chung [:steveck] from comment #12)
> I think the 1st patch is good enough since the main idea is thumbnail update
> without generating another iframe.

I'm still concerned about keeping the unresized undecoded blob in memory :/ I'll need to measure memory on the device and compare. NI me to do this measurement.

> The only concern if the heigh of iframe
> is not fixed while in preview mode, and you can see the iframe resized when
> image loaded/rendered. Since the image attachment size is fixed currently,
> maybe we could also set the iframe with fixed height to prevent unnecessary
> iframe resizing.

Yep, I agree, will look into it.

> 
> Sorry I haven't looked into your 2nd patch yet, but Jenny got some different
> thought about your image place holder idea after she saw your patch actually
> running on device, so ni? her here and I'll left more thought for both
> patch, thanks!

Comment 15

[Julien Wajsberg [:julienw] (PTO -> may 13)]

So, here are my measurements, done on Flame with b2g-info, using the column USS

The first measurement is when I'm sitting in the "New message" panel until the measurement is stable. I don't know why it's changing, but it helps showing the memory increase.
The second measurement is after attaching an attachment, when the measurement is stable.

before patch:
18   -> 19.7 (without cropping the image)
18.4 -> 19.8 (cropping the image)

after patch:
18   -> 20.6 (without cropping the image)
18.3 -> 21.3 (cropping the image)

The image was taken with the Flame's camera, its undecoded size is a little less than 1MB.

I did the measurement once only, so we need to be careful when analysing this precisely, but it seems clear that keeping the undecoded blob costs memory.

Now, maybe it is good enough for v2.0/v2.1, given we'll rarely have more than 5 image attachments, and we can do a better patch for master.

Comment 16

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Will file a separate bug for the more invasive solution.

Comment 17

[Jenny Lee]

Hi Julien,

Steve showed me both patch 1 and 2. I think patch 1 is a better fix for the flickering situation and I'd suggest we apply this fix to 2.0 ;) The reason is that the placeholder appears before actual picture also gives a flickering visual experience, as it is only present for under 0~1ish sec (from what I see).

We could potentially work on a solution based on your placeholder idea, for example replacing it with a spinner. Ideally what would happen is:

1. Attach a photo
2. Show spinner (during the time spinner is shown, do resizing)
3. Show thumbnail that's resized along with the size info

So we give user instant feedback on attach action by immediately show a spinner, the spinner sends a clear message that the device is undergoing a process, and then, when resize is done, we show the thumbnail directly as well as the updated size info. Let me know what you think!

Comment 18

[Josh Cheng [:josh]]

This is reported by partner but not major blocker in 2.0. Make it 2.0M+.

Comment 19

[Steve Chung [:steveck]]

Comment on attachment 8503308 [details] [review]
github PR

This solution should satisfy the old branches except the private method, thanks!

Comment 20

[Julien Wajsberg [:julienw] (PTO -> may 13)]

I updated the smaller patch but I still miss one test change.

Comment 21

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Comment on attachment 8503308 [details] [review]
github PR

Hey Steve, this is ready for your review.

I'll move the more involved version to a separate bug and I'll also try to finish it today.

Comment 22

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Comment on attachment 8504177 [details] [review]
github PR - more invasive

Moved to bug 1088526

Comment 23

[Steve Chung [:steveck]]

Comment on attachment 8503308 [details] [review]
github PR

Except some nits for iframe translation and unit test, overall looks great. r=me and thanks!

Comment 24

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Hey Steve,

I try to reduce the flickering a little more, I pushed a new version but it's not perfect yet. I'll probably ask you a new review monday :)

Comment 25

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Comment on attachment 8503308 [details] [review]
github PR

Hey Steve,

here is an improved patch !

I added 2 commits:
* 1 commit is handling your comments + 1 unit test to check we update the file size in iframe too
* 1 commit is removing the additional flickering we see when inserting the iframe. I now use a Blob URL instead of the previous mechanism (waiting for load + inserting using innerHTML). In the past we used a data URL because I think we didn't have the handy Blob constructor, but now we can do this in an easier way.
  I also adds the "preview"/"nopreview" class directly in the iframe markup because I still could see a visual change when we changed it. An alternative would have been to hide the iframe until we add this class, but I thought it was cleaner this way.
  No unit test changes for this because our tests are testing what's happening after the whole "render" operation. I could not think of any meaningful test for this change.

Tell me what you think!

Comment 26

[Steve Chung [:steveck]]

Comment on attachment 8503308 [details] [review]
github PR

This "preview"/"nopreview" class preset and css refinement is really great and it did improve the visual that avoid the flickering completely. However, I have a concern about the iframe blob url mechanism. I left a comment about this issue and I'm ok if you check the error in correct position and create a follow up for it.

Comment 27

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Comment on attachment 8503308 [details] [review]
github PR

Hey Steve,

you had totally right, I updated the patch using your suggestion.

It was quite difficult to ensure the frame with the correct content was loaded, I finally used the class I expect on document.body.

Tell me what you think!

Comment 28

[Steve Chung [:steveck]]

Comment on attachment 8503308 [details] [review]
github PR

I have no better idea currently. I expected that readyState might be changed after src path reset but seems not. Having a comment for this class checking might be good enough here, thanks!

Comment 29

[Julien Wajsberg [:julienw] (PTO -> may 13)]

fixed commit and landed in:
master: d8394ec06c77325e1aa1f0ed2214b0c3a1f402d2

Comment 30

[Julien Wajsberg [:julienw] (PTO -> may 13)]

(In reply to Julien Wajsberg [:julienw] from comment #29)
> fixed commit and landed in:
I meant: fixed comment :)

Comment 31

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Demo: https://www.youtube.com/watch?v=HQR4ll2pJk0

Comment 32

[Julien Wajsberg [:julienw] (PTO -> may 13)]

New demo: http://youtu.be/f3XD1lTRgAI

Comment 33

[Kai-Zhen Li [:seinlin][:kli]]

Uplift to v2.0m get conflicts.

Comment 34

[Kai-Zhen Li [:seinlin][:kli]]

Julien, Uplift from master to 2.0m get conflicts. If we need to land something to 2.0m for this bug. Could you help to rebase a patch for v2.0m? Thanks!

Comment 35

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Hey Kai-Zhen,

I'm very sorry that I missed your previous NI, I think I removed it by mistake and then I likely overlooked the mail about it :/

This is a significant work to make a 2.0m specific branch for this patch. This patch builds on several refactoring patches that landed previously (notably bug 1025552, but even that bug does not apply cleanly in 2.0m), and contains itself some refactoring to make it work. I tried doing it but it's really not easy. It's not "just a simple uplift".

As a result, we'd need to invest significant engineering work to make a patch, and it would have a significant risk too. We can see the patch on master got one regression (bug 1090821), which is a sign that it's risky.

As a result, I don't think we should uplift this to 2.0m, especially given that it's only polish and does not prevent any functionality from the product.

Please tell me your thoughts.

Comment 36

[Kai-Zhen Li [:seinlin][:kli]]

Josh, Per comment 35, do you think we still need to keep this bug as a woodduck blocker? In my opinion, I agree with Julien.

Comment 37

[Josh Cheng [:josh]]

Hi Julien, Hi Kai-Zhen,
I support your suggestion as this is non trivial fix and introduce regression issues which we should not fix this on 2.0M. 

However do you think it's possible to pick both this bug and bug 1090821 on 2.1 since 2.1 could be next version Woodduck will upgrade to.
Thanks!

Comment 38

[Norry.L.F(Leave from Mozilla)]

Attachment: Verify.mp4

This bug can be 100% reproed on Woodduck 2.0M and Flame 2.0/2.1/2.2

Attached: Verify.mp4&logcat.txt
Found at: 4:35

Woodduck build:
Gaia-Rev        8e17537f9e6d02b7271d92134671414d1fb88386
Gecko-Rev       71be912cc4fedd423869ad9dce72880de170851f
Build-ID        20141128050313
Version         32.0
Device-Name     jrdhz72_w_ff
FW-Release      4.4.2
FW-Incremental  1417122322
FW-Date         Fri Nov 28 05:05:45 CST 2014
------------------
Flame 2.0:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3
Build-ID        20141127000203
Version         32.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141127.034818
FW-Date         Thu Nov 27 03:48:29 EST 2014
Bootloader      L1TC00011880
------------------
FLame2.1 build:
Gaia-Rev        5372b675e018b6aac97d95ff5db8d4bd16addb9b
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/f34377ae402b
Build-ID        20141127001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141127.035005
FW-Date         Thu Nov 27 03:50:16 EST 2014
Bootloader      L1TC00011880
------------------
FLame2.2 build:
Gaia-Rev        80bc1445959db79e9d2e947cc56e1eb7b0d3d0f0
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/7dfad34d265b
Build-ID        20141127040204
Version         36.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141127.074535
FW-Date         Thu Nov 27 07:45:46 EST 2014
Bootloader      L1TC00011880

Comment 39

[Norry.L.F(Leave from Mozilla)]

Attachment: logcat.txt

Comment 40

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Yeah, I'm a lot more comfortable with uplifting this to v2.1.

Comment 41

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Norry, wrong bug, I guess.

Comment 42

[Norry.L.F(Leave from Mozilla)]

Attachment: Verify1.mp4

Hi Josh,

Here comes the verify video on woodduck 2.0m, please check.

Comment 43

[Josh Cheng [:josh]]

Triage:
Minor issue and fix in 2.1/2.2

Comment 45

[Shally Li（leave from Mozilla）]

This bug has been successfully verified on latest Flame v2.2.
See attachment: verified_v2.2.mp4
Reproduce rate: 0/5

STR:
1.Launch Messages app.
2.Tap "+" to create new message.
3.Tap attachment icon and select "Camera".
4.Take one picture,then tap "Select" button.
**The picture does not flicker in Messages app. --OK

Flame 2.2 (Pass):
Build ID               20150316162504
Gaia Revision          d0e09d5e6367e558824f9cbf691da99cedf63037
Gaia Date              2015-03-16 17:14:22
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/793d61bb0bd4
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150316.195035
Firmware Date          Mon Mar 16 19:50:48 EDT 2015
Bootloader             L1TC000118D0

Comment 46

[Shally Li（leave from Mozilla）]

Attachment: verified_v2.2.mp4