1064126 - Video app memory growth during updating thumbnails of different videos

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Video, defect, P1)

Description

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

STR :

1) Load 20 HD videos on device
2) Reboot device and launch video app.
3) Run below command :
adb shell "b2g-info && cat /sys/class/kgsl/kgsl/page_alloc && cat /proc/<video app pid>/status | grep VmSwap && cat /d/ion/iommu | grep 'total ' "
4) Play any video , pause it and return video list window on video app. Video app will update video list with new thumbnail where playback has stopped.
5) Run Command from step (3)
6) You will see Video app's total USS (USS in b2g-info and Vmswap value from |cat /proc/pid/status| ), kgsl memory usage and iON usage is increasing for each new thumbnail
7) Repeat step 3,4,5,6 for each of the 20 HD video files on sdcard. 
8) We will soon see video app started using at least additional 10MN memory.

Device used : Flame 256MB JB build. (You need to use mem=271m for FLAME device) 

it is 100% reproducible on FLAME 256MB device.

I think that video app is not clearing thumbnail memory of previous videos while storing new thumbnails after playing each video file.

Video app memory growth causes it to be killed by LMK in stability test and it is one of the reason why it is getting killed although it is ONLY foreground app on device.


FFOS v2.0 version:

https://www.codeaurora.org/cgit/quic/lf/b2g/mozilla/gaia/commit/?h=mozilla/v2.0&id=4627014cc5c5eeec894183866d4c57291302f8b8
https://www.codeaurora.org/cgit/quic/lf/b2g/mozilla/gecko/commit/?h=mozilla/v2.0&id=08a5d8c66bcacb07df1e9125ff24f9d0924ebe68


Please note if I try to use |get_about_memory.py| then video app memory usage comes down sometime but video app should release previous thumbnail caches even if we don't use |get_about_memory.py| as it affects device performance and video app is getting killed by LMK (in stability test) although it is only foreground app in device. 


I am not attaching any memory logs as it is reproducible on FLame always and I provided commands to see growth in step(3)

Comment 1

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

[Blocking Requested - why for this release]:

Comment 2

[Sotaro Ikeda [:sotaro]]

On Firefox OS, at most only one OMXCodec can be instantiated at the same time. Therefore, kgsl memory and ion should be related to gfx layers and canvas elements.

Comment 3

[Hema Koka [:hema]]

Blocking Reason: As stated in the original desc, "Video app memory growth causes it to be killed by LMK in stability test and it is one of the reason why it is getting killed although it is ONLY foreground app on device."

Comment 4

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Please note that I used 20 video files and each of the video file has 640x480 resolution and all video files is in MP4 format.

This issue is very easily reproducible on FLAME device (256MB). I verified it myself as I said in Comment 0.

Comment 5

[Sotaro Ikeda [:sotaro]]

I tried to reproduce the crash with 854*480 resolution vides on v2.0 flame JB(271MB). But I did not see the video app's crash until now.

I used a video of Bug 889238.

Comment 6

[Sotaro Ikeda [:sotaro]]

Tapas can you reproduce the problem by using video of Bug 889238?

Comment 7

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #6)
> Tapas can you reproduce the problem by using video of Bug 889238?

Can you see the crash by using the video?

Comment 8

[Sotaro Ikeda [:sotaro]]

By the way, the video thumbnail is rendered with some shear. This seems like a bug.

Comment 9

[Sotaro Ikeda [:sotaro]]

I am going to try again with HD videos.

Comment 10

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #9)
> I am going to try again with HD videos.

I saw the crash by using HD video. When sd card have a lot of HD videos, the video app becomes very unresponsive. It seem a problem that related to Bug 1013756.

Comment 11

[Sotaro Ikeda [:sotaro]]

> 
> I think that video app is not clearing thumbnail memory of previous videos
> while storing new thumbnails after playing each video file.

video app creates and uses canvas only within captureFrame(). The captureFrame() is called only when OMXCodec is loaded. 
https://github.com/mozilla-b2g/gaia/blob/v2.0/apps/video/js/metadata.js#L300

Comment 12

[Sotaro Ikeda [:sotaro]]

canvas is destroyed during GC. It seems to defer canvas's resources release.

Comment 13

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(In reply to Sotaro Ikeda [:sotaro] from comment #12)
> canvas is destroyed during GC. It seems to defer canvas's resources release.

It seems like you found root cause of memory growth. Do you still need any help from me ? You don't need to use HD video. Use any video of (640x380) resolution and don't try to crash video app. Please try to run STR from Comment 0 and see memory growth by executing command from Comment 0. 

Please make NI on me if you still need any help from me :) Thanks a lot for help

Comment 14

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(In reply to Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me) from comment #13)
> Use any video of (640x380)
> resolution and don't try to crash video app.

it should be 640x480 .. Sorry for the mistake.

Comment 15

[Sotaro Ikeda [:sotaro]]

(In reply to Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me) from comment #13)
> 
> Please make NI on me if you still need any help from me :) Thanks a lot for
> help

I seems to found it. Help is not needed now. Thank!

Comment 16

[Sotaro Ikeda [:sotaro]]

canvas's native side implementation is CanvasRenderingContext2D. When CanvasRenderingContext2D allocates a rendering target, it is alive until CanvasRenderingContext2D's destructor. Other case of releasing the rendering target is "Change canvas dimension" or "Change moz-opaque attribute".

Comment 17

[Sotaro Ikeda [:sotaro]]

Attachment: temporay log patch

The patch that I used to check number of canvas instances.

Comment 18

[Sotaro Ikeda [:sotaro]]

When canvas is rendered by SkiaGL, SurfaceStream is used for rendering to TextureClient. The SurfaceStream allocates ion(TextureClient).

Comment 19

[Sotaro Ikeda [:sotaro]]

By using attachment 8487363 [details] [diff] [review], I confirmed that the numbers of canvas and SurfaceStream were risen up to 8.

Comment 20

[Sotaro Ikeda [:sotaro]]

gaia side does not have a api to request canvas resource release. "Change canvas dimension" or "Change moz-opaque attribute" could be a workaround.

Comment 21

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Change canvas size when end of use

Comment 22

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Change canvas size when end of use

Remove unnecessary change.

Comment 23

[Sotaro Ikeda [:sotaro]]

attachment 8487375 [details] [diff] [review] decreases the max number of SurfaceStream from 8 to 2.

Comment 24

[Sotaro Ikeda [:sotaro]]

hema, can you assign a gaia engineer? Just update attachment 8487375 [details] [diff] [review] correctly seems enough. Though I am not sure about gaia side.

Comment 25

[Sotaro Ikeda [:sotaro]]

Tapas, can you check if attachment 8487375 [details] [diff] [review] fix the problem for you?

Comment 26

[Hema Koka [:hema]]

David/Russ, 

Can one of you please take this up today and try Sotaro's fix suggestion?

Thanks
Hema

Comment 27

[David Flanagan [:djf]]

Thanks for investigating this, Sotaro. In Gallery there are a number of places where we set the canvas width to 0 after using it to release memory. I guess we forgot to do that in the Video app. I'll take this and see if I can finish it up.

Comment 28

[David Flanagan [:djf]]

Attachment: link to patch on github

Russ,

This is my variant of the basic patch Sotaro proposed. Note that I change it so that the canvas size is not cleared until after toBlob() is done, which seems more robust to me.

I've basically copied what we do in Gallery here and have included a couple of other changes to the way the canvas is initialized. These are things I learned about for gallery after this captureFrame() method had been written. It is better to set the canvas size before creating the context because it prevents memory reallocations, for example.  And the willReadFrequently flag is a hint to keep the canvas off of the GPU.

If you are able to reproduce the bug, please check the code and verify that it fixes the bug.

Comment 29

[Russ Nicoletti [:russn]]

Comment on attachment 8487546 [details] [review]
link to patch on github

Without applying any patches, the total memory from following the STR in the original description was 10072064.

After applying the patch from dfj, the total memory following the same steps was 6307840. 

Therefore, I'm concluding the patch is properly releasing memory from the canvas after viewing a video.

Comment 30

[David Flanagan [:djf]]

Landed on master: https://github.com/mozilla-b2g/gaia/commit/a5949ec98cf3ed7bb46d8c7b13678ff3c767c71c

Comment 31

[David Flanagan [:djf]]

Uplifted to v2.1: https://github.com/mozilla-b2g/gaia/commit/7f8fcee7c4e57370b176125ef734d76b5ddcd234

Comment 32

[David Flanagan [:djf]]

Uplifted to v2.0: https://github.com/mozilla-b2g/gaia/commit/cd198d77669c9d283b0e2afe3f13e294b2561ac5

Comment 33

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(In reply to David Flanagan [:djf] from comment #32)
> Uplifted to v2.0:
> https://github.com/mozilla-b2g/gaia/commit/
> cd198d77669c9d283b0e2afe3f13e294b2561ac5

This definitely saves a lot memory but I am still very small growth. I will run automation for 24 hours to see if it increases more or not. Thanks for help !

Comment 34

[Alissa（leave from Mozilla）]

This issue has been successfully verified on Flame 2.0,2.1, 2.2
See attachment:verify.png
Reproducing rate: 0/5

Flame 2.1 new versions:
Gaia-Rev        ccb49abe412c978a4045f0c75abff534372716c4
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18fb67530b22
Build-ID        20141130001203
Version         34.0
Flame 2.0 new versions:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3
Build-ID        20141130000204
Version         32.0
Flame 2.2 new version:
Gaia-Rev        7119da7a86cd803840678ca3a6067e5622adc481
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/df3fc7cb7e80
Build-ID        20141130040207
Version         37.0a1

Comment 35

[Alissa（leave from Mozilla）]

(In reply to Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me) from comment #33)
> (In reply to David Flanagan [:djf] from comment #32)
> > Uplifted to v2.0:
> > https://github.com/mozilla-b2g/gaia/commit/
> > cd198d77669c9d283b0e2afe3f13e294b2561ac5
> 
> This definitely saves a lot memory but I am still very small growth. I will
> run automation for 24 hours to see if it increases more or not. Thanks for
> help !

Hi tapas,
The growth is small,and uss fluctuate in 4M,sometimes it reduce to the same with first time playing,So I think it is right,We do not test it for 24 hours,if there is problem please contact with me.

Comment 36

[Alissa（leave from Mozilla）]

Attachment: flame 2.2

Comment 37

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(In reply to Alissa from comment #35)
> (In reply to Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me) from
> comment #33)
> > (In reply to David Flanagan [:djf] from comment #32)
> > > Uplifted to v2.0:
> > > https://github.com/mozilla-b2g/gaia/commit/
> > > cd198d77669c9d283b0e2afe3f13e294b2561ac5
> > 
> > This definitely saves a lot memory but I am still very small growth. I will
> > run automation for 24 hours to see if it increases more or not. Thanks for
> > help !
> 
> Hi tapas,
> The growth is small,and uss fluctuate in 4M,sometimes it reduce to the same
> with first time playing,So I think it is right,We do not test it for 24
> hours,if there is problem please contact with me.

Yeah agreed.. This bug can be closed if fixes has already landed :) . Sorry for delayed update.