1157616 - [FFOS7715 v2.1][2.1s][camera][layerscope]Recording video, then play the video, at the end of the video, layer-compress has some problem.

Status: RESOLVED WONTFIX

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

Description

[jinchao.wang]

Description:Recording video, then play the video, at the end of the video, layer-compress has some problem.


Steps to Reproduce:
1) open camera to recording a video
2) click thumbnail to play video
3) When video play to the end , it will switch to poster interface. 

Actual Result：
But the poster interface refresh abnormally, the layer-compress has some problem.


Repro frequency：8/10

Analysis:
Camera uses shared/js/media/media_frame.js and video_player.js to play video.
  media_frame.js:
    var poster = newelt(container, 'img', 'videoPoster');
    var player = newelt(container, 'video', 'videoPlayer');
    var controls = newelt(container, 'div', 'videoPlayerControls');
    ……

Comment 1

[jinchao.wang]

Attachment: play_normal.png

Comment 2

[jinchao.wang]

Attachment: end_draw_half-baked .png

Texture3,4,11 don't decode anything.

Comment 3

[jinchao.wang]

Attachment: end_draw_whole.png

Comment 4

[jinchao.wang]

Flame has this problem too.

I don't think it's camera's problem. It can reproduce in both gallery and camera.

Comment 5

[jinchao.wang]

Hi Vance:
   Can you arrange some one to optimize this problem? thanks very much!

Comment 6

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Hum....not quite sure i understand what is the issue here. Jinnchao is it possible you can provide a reproduce video.

ni Vincent first to see if he has any comment

Thanks

Comment 7

[jinchao.wang]

Attachment: issue happened at 3rd seconds

Comment 8

[jinchao.wang]

(In reply to Vance Chen [:vchen][[e-mail]] from comment #6)
> Hum....not quite sure i understand what is the issue here. Jinnchao is it
> possible you can provide a reproduce video.
> 
> ni Vincent first to see if he has any comment
> 
> Thanks

Hi Vance:
    I has uploaded the video about the issue. thanks.

Comment 9

[Vincent Liu[:vliu]]

Current result.

1. Flame on v2.1 branch also has this issue.
2. There is no such an issue on Master branch.
3. If I played this recorded video clip in Gallery app, I can also see this problem

After talked with Gary, and he'd ever met similar gaia issues before. ni? Gary for further study.

Comment 10

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

Hi Vliu,
   Please refer to following code[1], it shows how to show/hide poster. Gaia will release 'src' for release memory every times.

   [1]https://github.com/mozilla-b2g/gaia/blob/master/shared/js/media/video_player.js#L172

Comment 11

[jinchao.wang]

Dear GaryChen & Vincent :
   Did there any update ? Did we apply some animation not appropriate when switch view ?

Comment 12

[Vincent Liu[:vliu]]

From more detailed investigation, it seems that the width/height of mDecoded of [1] affects the drawing area when posting this image. It might be affected by partial decoding for a jpeg file. 

[1] : https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/file/228ca07a59b9/image/src/imgFrame.cpp#l331

When Camera app runs showPoster(), the first calling of imgFrame::SurfaceForDrawing() got the below size for mDecoded. 

(gdb) p mDecoded
$6 = {<mozilla::gfx::BaseRect<int, nsIntRect, nsIntPoint, nsIntSize, nsIntMargin>> = {x = 0, y = 0, width = 1280, height = 120}, <No data fields>}

I will then do more observations.

Comment 13

[Vincent Liu[:vliu]]

Attachment: image-partial-display.jpg

I tried to set two break points on flame v2.1 branch for observation. Please see [1][2].

[1]: https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/file/b5d2472c2940/image/src/imgFrame.cpp#l363

[2]: https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/file/b5d2472c2940/gfx/thebes/gfxUtils.cpp#l621

When Camera app ends up with playing the recording video, it start to show poster image. From my current observation, the size of mDecoded in Comment 12 affects the drawing region in [2] and that is the reason why we can see partial display for image.

Hi Seth,

Based on the above observation, could you please have some comments for this? Master branch doesn't have this issue so it might have bug to fix this. If you know about this bug, is it possible to cherry pick to v2.1s? Or you may recommend someone else to help me if you are not the proper one. Really thanks.

Comment 14

[Seth Fowler [:seth] [:s2h]]

Vincent, I'm happy to try to help, though I can only guess with the information in this bug.

mDecoded is just the region of the image that has been decoded so far. So it sounds like when we attempt to draw the poster image for the first time, we haven't finished decoding it. That's perfectly normal. But it seems like we aren't redrawing the poster image when it finishes decoding - I suspect that's the bug here.

I'm not sure how the poster image is implemented internally. If an nsImageFrame is created to display it, perhaps this is the same issue discussed in bug 1130328?

If that's not it, I'm not sure. Image code changed a _lot_ between B2G 2.1 and trunk. You might need to do some bisecting to try to narrow down when this got fixed.

Comment 15

[Vincent Liu[:vliu]]

(In reply to Seth Fowler [:seth] from comment #14)
> Vincent, I'm happy to try to help, though I can only guess with the
> information in this bug.
> 
> mDecoded is just the region of the image that has been decoded so far. So it
> sounds like when we attempt to draw the poster image for the first time, we
> haven't finished decoding it. That's perfectly normal. But it seems like we
> aren't redrawing the poster image when it finishes decoding - I suspect
> that's the bug here.

Really thanks for your reply. I tried to dump mDecoded in [3][4] in both master and v2.1 branch for drawing.

@@@ Master branch:
[3]: https://hg.mozilla.org/mozilla-central/file/4b9b12c248dc/image/src/imgFrame.cpp#l576

I/Gecko   ( 2239):  imgFrame::SurfaceForDrawing. mDecoded.width=0, mDecoded.height=0 
I/Gecko   ( 2239):  imgFrame::SurfaceForDrawing. mDecoded.width=1280, mDecoded.height=720

@@@ v2.1 branch:
[4]: https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/file/b5d2472c2940/image/src/imgFrame.cpp#l331

I/Gecko   ( 2239):  imgFrame::SurfaceForDrawing. mDecoded.width=1280, mDecoded.height=112 
I/Gecko   ( 2239):  imgFrame::SurfaceForDrawing. mDecoded.width=1280, mDecoded.height=384

Does it means image decoding on master branch is not partial decoding? Or waiting until the image decoding is done?

> I'm not sure how the poster image is implemented internally. If an
> nsImageFrame is created to display it, perhaps this is the same issue
> discussed in bug 1130328?
>

I will try to patch into v2.1 to see if it is the same issue.

Comment 16

[Seth Fowler [:seth] [:s2h]]

(In reply to Vincent Liu[:vliu] from comment #15)
> Does it means image decoding on master branch is not partial decoding? Or
> waiting until the image decoding is done?

We still do partial decoding on the master branch. However, on the master branch we will decode the entire image at once (off-main-thread) if we have all the data available. On older Gecko versions, like v2.1 is using, we decode an image in chunks, even if all the data is available. That changes the timing of all sorts of things, and I suspect that's why you're seeing a difference in what mDecoded looks like on the two branches.

Comment 17

[Vincent Liu[:vliu]]

(In reply to Seth Fowler [:seth] from comment #14)

> I'm not sure how the poster image is implemented internally. If an
> nsImageFrame is created to display it, perhaps this is the same issue
> discussed in bug 1130328?
> 

The patch of bug 1130328 doesn't fix this problem.

> We still do partial decoding on the master branch. However, on the master
> branch we will decode the entire image at once (off-main-thread) if we have
> all the data available. On older Gecko versions, like v2.1 is using, we
> decode an image in chunks, even if all the data is available. That changes
> the timing of all sorts of things, and I suspect that's why you're seeing a
> difference in what mDecoded looks like on the two branches.

If your suspicion is right, is it possible to cherry-pick some other patches to fix it? Or do you have any idea to fix it only on v2.1s branch? I'd tried to have some hacks to fix it, but I am not sure there is any potential risk. Or you may have better idea to enhance it. Really thanks

diff --git a/image/decoders/nsJPEGDecoder.cpp b/image/decoders/nsJPEGDecoder.cpp
index 77b517b..3c63b1a 100644
--- a/image/decoders/nsJPEGDecoder.cpp
+++ b/image/decoders/nsJPEGDecoder.cpp
@@ -664,7 +664,8 @@ nsJPEGDecoder::OutputScanlines(bool* suspend)
       }
   }
 
-  if (top != mInfo.output_scanline) {
+  // if (top != mInfo.output_scanline) {
+  if (mInfo.output_scanline >= mInfo.output_height) {
       nsIntRect r(0, top, mInfo.output_width, mInfo.output_scanline-top);
       PostInvalidation(r);
   }

Comment 18

[Vincent Liu[:vliu]]

Hi Seth,

According to Comment 17, do you have any idea? Thanks

Comment 19

[Seth Fowler [:seth] [:s2h]]

Vincent, I think you are focusing on the wrong thing here. Right now I see no evidence that imgFrame::mDecoded is being computed wrong or that there's any problem related to it. If you want to try to debug this, you need to figure out where the poster frame is handling imgINotificationObserver::FRAME_UPDATE messages, and make sure that:

(1) The invalidations we're sending union to cover the entire image. You'll want to look at the nsIntRect parameter to Notify to determine this.

(2) We're actually repainting in response to those invalidations.

(It's possible that the poster frame just waits for imgINotificationObserver::FRAME_COMPLETE; check that too.)

However, debugging this may be a waste of time. If this is fixed on master, I think it'd be a better use of your time to do some bisecting and figure out which commit fixed it.

Comment 20

[Vincent Liu[:vliu]]

(In reply to Seth Fowler [:seth] from comment #19)

> However, debugging this may be a waste of time. If this is fixed on master,
> I think it'd be a better use of your time to do some bisecting and figure
> out which commit fixed it.

Hi :seth,

Really thanks for your suggestion. I will try to get a reverse regression window here. 

As the current information, this bug has been fixed on master. It needs to bisecting and figure out which commit fixed it.

Comment 21

[Pi Wei Cheng [:piwei] (inactive)]

I'll work on getting the reverse window.

Comment 22

[Pi Wei Cheng [:piwei] (inactive)]

b2g-inbound reverse regression window:

Last Broken
Device: Flame
BuildID: 20140626101147
Gaia: 1f0902c6165297d51883ca6f9fe300bfee1f4c08
Gecko: f9c125b68bb4
Version: 33.0a1 (2.1 Master) 
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

First Working
Device: Flame 2.1
BuildID: 20140626104247
Gaia: 77b09fdc169c34f285ed92090f090a2d3e9bf688
Gecko: 3658f3306eb0
Version: 33.0a1 (2.1 Master) 
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

Last Broken Gaia First Working Gecko - issue DOES repro
Gaia: 1f0902c6165297d51883ca6f9fe300bfee1f4c08
Gecko: 3658f3306eb0

Last Broken Gecko First Working Gaia - issue does NOT repro
Gaia: 77b09fdc169c34f285ed92090f090a2d3e9bf688
Gecko: f9c125b68bb4

Gaia pushlog:
https://github.com/mozilla-b2g/gaia/compare/1f0902c6165297d51883ca6f9fe300bfee1f4c08...77b09fdc169c34f285ed92090f090a2d3e9bf688

This issue was fixed on 2.1 master by the patch for bug 1014968. It looks like something else broke this AFTER it had branched off to 2.1 (b2g34). For now we can request uplifting to v2.1, but if not we can always find out what actually caused it in b2g34 branch.

Comment 23

[KTucker [:KTucker][Inactive 3/4/2016]]

Justin, can you take a look at this please? This was originally fixed by the work done for bug 1014968 on 2.1 when it was master. Can we get this uplifted again to 2.1 since this broke again?

Comment 24

[Andrew Osmond [:aosmond] (he/him)]

This appears to be a conflict of fixes between bug 1108979 and bug 1014968.

Fix for the issue tracked in this bug and 1014968:
https://github.com/mozilla-b2g/gaia/commit/ae9f6fe87ee39e2bdc240f175d672fa74d3e3627

And it was undone in bug 1108979:
https://github.com/mozilla-b2g/gaia/commit/a7ae581a9e5cdcdfae08c92f63aa571fc024bdde

Comment 25

[Justin D'Arcangelo [:justindarc]]

(In reply to KTucker [:KTucker] from comment #23)
> Justin, can you take a look at this please? This was originally fixed by the
> work done for bug 1014968 on 2.1 when it was master. Can we get this
> uplifted again to 2.1 since this broke again?

I have asked Andrew to take a look at this. If he is unable to resolve, you can re-NI? me and I'll take a look. Thanks!

Comment 26

[GitHub Autolander Bot]

Attachment: [gaia] aosmond:bug1157616-v2.1 > mozilla-b2g:v2.1

Comment 27

[GitHub Autolander Bot]

Attachment: [gaia] aosmond:bug1157616 > mozilla-b2g:master

Comment 28

[Andrew Osmond [:aosmond] (he/him)]

The master and 2.1 PRs are identical. Even though the reported issue doesn't exist on master, I believe the existing logic is wrong. It causes the poster image to be gray briefly when showing again after ending the play because we cleared the cached values too early; we should release them when we exit the view for that particular video instead.

Neither bug 1108979 nor bug 1014968 should reproduce now.

Comment 29

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8608925 [details] [review]
[gaia] aosmond:bug1157616-v2.1 > mozilla-b2g:v2.1

Switching R? to djf since this patch is for shared/js/media/video_player.js.

Comment 30

[David Flanagan [:djf]]

(In reply to Andrew Osmond [:aosmond] from comment #28)
> The master and 2.1 PRs are identical. Even though the reported issue doesn't
> exist on master, I believe the existing logic is wrong. It causes the poster
> image to be gray briefly when showing again after ending the play because we
> cleared the cached values too early; we should release them when we exit the
> view for that particular video instead.
> 
> Neither bug 1108979 nor bug 1014968 should reproduce now.

Pretty funny that the two bugs have exactly opposite fixes. I never noticed that.

Am I understanding correctly that this bug is about a bit of gray that shows for a fraction of a second while the poster image is being decoded again? It doesn't seem like a big deal to me.

The code to discard the image when it is not displayed dates back to the time when we had OOMs all of the time and it was important to save image memory whenever possible. So my impulse is still to discard it, and prefer a fix that just hides the graphical jank with a CSS opacity transition or a setTimeout before hiding the video element or something.

Note that in master, just hiding the image with display:none is enough that the decoded image might be released. In general, there is no way to hide the image and guarantee that it will remain decoded for quick re-display, so I'm not sure that it matters much either way on master.

Let me know how urgent it is to get this reviewed. I've got 2.2+ blockers that I'm working on currently, so unless you need this review ASAP it might not happen until next week.

Comment 31

[Andrew Osmond [:aosmond] (he/him)]

(In reply to David Flanagan [:djf] from comment #30)
> (In reply to Andrew Osmond [:aosmond] from comment #28)
> > The master and 2.1 PRs are identical. Even though the reported issue doesn't
> > exist on master, I believe the existing logic is wrong. It causes the poster
> > image to be gray briefly when showing again after ending the play because we
> > cleared the cached values too early; we should release them when we exit the
> > view for that particular video instead.
> > 
> > Neither bug 1108979 nor bug 1014968 should reproduce now.
> 
> Pretty funny that the two bugs have exactly opposite fixes. I never noticed
> that.
> 
> Am I understanding correctly that this bug is about a bit of gray that shows
> for a fraction of a second while the poster image is being decoded again? It
> doesn't seem like a big deal to me.
> 
> The code to discard the image when it is not displayed dates back to the
> time when we had OOMs all of the time and it was important to save image
> memory whenever possible. So my impulse is still to discard it, and prefer a
> fix that just hides the graphical jank with a CSS opacity transition or a
> setTimeout before hiding the video element or something.
> 
> Note that in master, just hiding the image with display:none is enough that
> the decoded image might be released. In general, there is no way to hide the
> image and guarantee that it will remain decoded for quick re-display, so I'm
> not sure that it matters much either way on master.
> 
> Let me know how urgent it is to get this reviewed. I've got 2.2+ blockers
> that I'm working on currently, so unless you need this review ASAP it might
> not happen until next week.

I don't think it is urgent. To me, it sort of looks bad but it is only for a brief moment and the functionality is not broken (UX/UI focused people may disagree however.) The OOM reasoning makes sense to me, I didn't really think about it.

Comment 32

[David Flanagan [:djf]]

Comment on attachment 8608925 [details] [review]
[gaia] aosmond:bug1157616-v2.1 > mozilla-b2g:v2.1

Sorry this review took so long. It got buried under a bunch of other stuff.

r+, though I'm not sure it is actually worth uplifting unless there is a partner that wants it.

Since the bug has not been reported on master, let's not land it to master now. There are various gecko imagelib changes landing, and I don't want to complicate things by changing gaia as well.

Comment 33

[BMO Automation]

Firefox OS is not being worked on