802647 - [BrowserAPI] Return screenshots as blobs instead of as base-64 strings

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Justin Lebar (not reading bugmail)]

A base-64 string inside our JS engine uses 16 bits of storage (one jschar) to encode 6 bits of data.  So switching from base-64 strings to blobs for screenshots will be a reduction of memory usage by a factor of 6/16 = .375.

I'd like us not to do this until we have good memory reporting of blobs, however.  We now have excellent memory reporting of long strings (bug 801780), which gives us insight into places where we're keeping screenshots around for too long (for whatever reason).

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

It would be a very good news if we can use screenshots without wasting memories!

Comment 2

[Justin Lebar (not reading bugmail)]

This is only one piece of the puzzle, as I'll explain in bug 798002 in a moment.  This bug is /not/ a free pass to take as many screenshots as you want.  :)

Comment 3

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v1

I think this works, but before we can land this, we need to test the Gaia stuff and make sure that it doesn't leak (the semantics with blob URLs are tricky).  And before we can determine whether it's leaking, we need about:memory tracking for these blobs (if we don't have that yet).

Comment 4

[Justin Lebar (not reading bugmail)]

Incomplete pull request to Gaia: https://github.com/mozilla-b2g/gaia/pull/5888

I need some help on the window manager part, and I need to add support for these blobs in about:memory so we can tell whether or not we're leaking (which is even easier to do with these blobs).

Comment 5

[Justin Lebar (not reading bugmail)]

[basecamp-blocking request]
I'd been saying that you could follow the dependency chain back to basecamp-blocking+ bug, but in fact bug 798002 is not blocking...yet.  Anyway, if that blocks, this blocks.

Comment 6

[Justin Lebar (not reading bugmail)]

Tim says we can land this once bug 801306 lands.  Landing this at that point will break some Gaia screenshotting functionality, but not all of it, and Tim can fix up the breakage once we land this.

Comment 7

[Justin Lebar (not reading bugmail)]

Attachment: Pull request placeholder

I need a review on the PR, but asking in github wasn't getting me anywhere...

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This is one of the 2 or 3 remaining "different phone" wins we have left before having to be clever.

Ben and Tim, please treat this work as highest priority.

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #8)
> This is one of the 2 or 3 remaining "different phone" wins we have left
> before having to be clever.
> 
> Ben and Tim, please treat this work as highest priority.

I already have patch part 1 ready for bug 801306, part 2 coming and it's a soft blocker to this one. Patch part 1 is pending a? from vivien so please ping him for that.

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Justin Lebar [:jlebar] from comment #7)
> Created attachment 674730 [details] [review]
> Pull request placeholder
> 
> I need a review on the PR, but asking in github wasn't getting me anywhere...

I am not aware you are looking for review? until now.

r+ for the cards_view.js part, however you would need to rebase your work before Ben can merge it on Github.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #9)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #8)
> > This is one of the 2 or 3 remaining "different phone" wins we have left
> > before having to be clever.
> > 
> > Ben and Tim, please treat this work as highest priority.
> 
> I already have patch part 1 ready for bug 801306, part 2 coming and it's a
> soft blocker to this one. Patch part 1 is pending a? from vivien so please
> ping him for that.

Why is it a soft blocker of this work?

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #9)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #8)
> > This is one of the 2 or 3 remaining "different phone" wins we have left
> > before having to be clever.
> > 
> > Ben and Tim, please treat this work as highest priority.
> 
> I already have patch part 1 ready for bug 801306, part 2 coming and it's a
> soft blocker to this one. Patch part 1 is pending a? from vivien so please
> ping him for that.

Why is bug blocking-basecamp- if this is a high priority? This is mainly why I am not looking at it closely. I believe it should be a blocker if it blocks a blocker. I will change that.

Comment 13

[Justin Lebar (not reading bugmail)]

Comment on attachment 674730 [details] [review]
Pull request placeholder

I've updated the PR to include the requisite window manager changes.

Things aren't silky-smooth as we transition between the live app and its screenshots; there are some jarring bumps.  But I don't think that's caused by this change, and if it is, I'm definitely not the person to fix or understand it!

Aside from that, everything seems fine.  Screenshots in the browser, the cards view, and window transitions all appear to work.  I can't guarantee that this doesn't leak screenshots (it's a lot easier to leak blobs because they're manually refcounted), but it doesn't seem to...

Comment 14

[Justin Lebar (not reading bugmail)]

Hm, I forgot that I have tests here that need to be updated.

I'll attach fixes soon.

Comment 15

[Justin Lebar (not reading bugmail)]

Attachment: Test fixes, v1

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Vivien Nicolas (:vingtetun) from comment #12)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #9)
> > (In reply to Chris Jones [:cjones] [:warhammer] from comment #8)
> > > This is one of the 2 or 3 remaining "different phone" wins we have left
> > > before having to be clever.
> > > 
> > > Ben and Tim, please treat this work as highest priority.
> > 
> > I already have patch part 1 ready for bug 801306, part 2 coming and it's a
> > soft blocker to this one. Patch part 1 is pending a? from vivien so please
> > ping him for that.
> 
> Why is bug blocking-basecamp- if this is a high priority? This is mainly why
> I am not looking at it closely. I believe it should be a blocker if it
> blocks a blocker. I will change that.

We don't have the right way to express this using our bug flags.

But from now on I guess I'll just not split hairs and (ab)use bb for this.

Comment 17

[Justin Lebar (not reading bugmail)]

Comment on attachment 676244 [details] [diff] [review]
Test fixes, v1

Dale is doing this review for me.

Comment 18

[Dale Harvey (:daleharvey)]

Comment on attachment 676244 [details] [diff] [review]
Test fixes, v1

looks great

Comment 19

[Justin Lebar (not reading bugmail)]

I've used up my quota for burning the tree for the month, so here we go:

https://tbpl.mozilla.org/?tree=Try&rev=8844b36ecc0f

Comment 20

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea64bf0290d5

Comment 21

[Justin Lebar (not reading bugmail)]

We need to land this PR when this cset hits central.  And we should land on Aurora soon thereafter.

https://github.com/mozilla-b2g/gaia/pull/5888

I'll do this if I'm awake, but my blessing to anyone who gets to it before me.

If Gaia and Gecko are out of sync here, screenshots won't work properly.  In particular, the cards view breaks pretty hard, not just visually, but also functionally.

Comment 22

[Ed Morley [:emorley]]

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

Comment 23

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1d407736951c