854795 - decode and downsample images at the same time for memory savings

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P1)

Description

[David Flanagan [:djf]]

The FirefoxOS Gallery app has serious problems with memory usage and crashes frequently with OOM errors when dealing with 5 megapixel images on low-memory devices.

It would be really nice if there was a way for imagelib to downsample an image while decoding it so that it could use less memory.

For example, say I've got a 3000x4000 photo. That's 12mp, and it takes 48mb of ram to decode.  But say that all I want to do is create a 300x400 preview image for that photo.  I'd like to be able to do this (or something equivalent with a different API):

  var image = new Image();
  image.width = 300;
  image.height = 400;
  image.src = URL.createObjectURL(blob);
  
Since I'm declaring the image to be just .12 megapixels large, I'd like to be able to only allocate something like .48 megabytes during the decoding process.

I understand that the decoding would have to be done again if I ever tried to enlarge that image, but having that kind of control over image memory usage would be a *huge win* in FirefoxOS. 

Without any mechanism like this, the Gallery app simply cannot open images larger than 5mp. So if someone transfers a 8mp photo from their phone to a FirefoxOS phone via bluetooth, FirefoxOS will refuse to display the photo.  It is not good.

Comment 1

[Justin Lebar (not reading bugmail)]

I'm pretty sure our libjpeg supports this, with the scale_num param.  We can scale to M/8 for all M from 1 to 16.  So in particular we can decode to 1/8 size.  That should be big enough for most use-cases, I'd think.  We'd then re-scale the small image to our actual size.

(1/8 size means each dimension gets divided by 8, so the memory usage is divided by 64.  So supposing we have a memory envelope of 10mb, how large of an image could we decode?

64 * (10 * 2^20 bytes) / (4 bytes / pixel) = ~168mp

A square 168mp image is roughly 13000 x 13000 px, which ought to big enough for our purposes.)

Comment 2

[David Flanagan [:djf]]

It would be so awesome if we could expose this capability. It is really kind of embarrassing (and partners keep reporting it as a bug) that we can't display photos from ordinary digital cameras in the gallery app because they are too large.

Maybe its too greedy (or too late), but I'm nominating this for leo. If not, then we should really do it for the version after that.

I'd guess that exposing this the way I ask in comment 0 actually runs the risk of breaking existing web content and that we might not want to attempt that...

What if we just expose scale_num through a mozscale attribute on the img element. And if the image was scaled when decoded, it would lie about its naturalWidth and naturalHeight and just report that it is smaller than it actually is? Or maybe we'd want new attributes mozScaledWidth and mozScaledHeight to go along with the mozscale attribute? (For the gallery app, I've already written a jpeg parser in javascript to determine the actual image size. So I know what the full image size is and I'm fine with reusing naturalWidth and naturalHeight.)

Comment 3

[Justin Lebar (not reading bugmail)]

> Maybe its too greedy (or too late), but I'm nominating this for leo.

This would be a fairly big change.  Moreover, imagelib just underwent a large refactoring for multithreaded image decoding, so I suspect backporting this to 18 would be difficult.

Comment 4

[David Flanagan [:djf]]

Is there any way at all to decode an image without decoding the entire thing at full size?

If I load an image into an offscreen image and then do a canvas.drawImage() on only a portion of the image, for example, will it decode just the rectangle I specify?

Comment 5

[Justin Lebar (not reading bugmail)]

> If I load an image into an offscreen image and then do a canvas.drawImage() on only a portion of the 
> image, for example, will it decode just the rectangle I specify?

I seriously doubt it, but I've been surprised before.

Comment 6

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

(In reply to David Flanagan [:djf] from comment #4)
> Is there any way at all to decode an image without decoding the entire thing
> at full size?
> 
> If I load an image into an offscreen image and then do a canvas.drawImage()
> on only a portion of the image, for example, will it decode just the
> rectangle I specify?

This would give you a clipped, rather than scaled, but I sense there is another question coming if the answer was yes?  Which I don't think it is, by the way :-)

Comment 7

[Alex Keybl [:akeybl]]

(In reply to David Flanagan [:djf] from comment #0)
> The FirefoxOS Gallery app has serious problems with memory usage and crashes
> frequently with OOM errors when dealing with 5 megapixel images on
> low-memory devices.

Leo Gaia QA - can you test to see whether the latest pre-production hardware runs into these issues when viewing photos taken by the internal camera?

Comment 8

[Alex Keybl [:akeybl]]

(please set blocking-b2g:leo? again if it does)

Comment 9

[David Flanagan [:djf]]

Photos taken by our camera app should have an internal preview image that avoids these issues.  The problem is when people try to test the phone (as they all seem to do) by throwing random photos that do not come from our camera on to it.

(Note also that as the screen size changes, we may run into problems where the preview image embedded by the camera is not big enough to fill the screen, so the gallery app will try to create its own preview and will have memory problems again.)

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to David Flanagan [:djf] from comment #4)
> Is there any way at all to decode an image without decoding the entire thing
> at full size?

Not as far as I know.

> If I load an image into an offscreen image and then do a canvas.drawImage()
> on only a portion of the image, for example, will it decode just the
> rectangle I specify?

Definitely not. Not if you changed the canvas transform appropriately, either.

It would be nice to be able to decode to the smaller size automatically based on how the image is being used, but I don't see a good way to do that.

I think a reasonable way to expose this would be to use media fragments, i.e. by extending http://www.w3.org/TR/media-frags. E.g. we could support URIs of the form "blahblah.jpg#scalewh=NNNxMMM" to scale to NNNxMMM in the decoder, so that's the decoded "natural height" of the image as far as the rest of the Web platform goes.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(and that would be reasonably easy to implement in imglib, I guess, taking advantage of that libjpeg feature)

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You'd probably want to define that it's an aspect-ratio-preserving scale so that the actual image size is the original size scaled so that it just contains NNNxMMM, i.e. it could be bigger in one dimension.

Comment 13

[Justin Lebar (not reading bugmail)]

What's the advantage of using media fragments as opposed to modifying the img size?  I presume it's because we already have image size that the media fragments spec concerns itself only with clipping.

Comment 14

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

The image size is an output from the decoder; you can't really modify it, just modify the area into which it's drawn. (Ignoring SVG, which of course works totally differently.) Media fragments, on the other hand, can be an _input_ to the decoder, which is why they're suitable for this task. I had considered the same idea as roc.

Note that we have already added a (Gecko-only for the time being) media fragment to select a desired size for icons - see bug 419588. It's worth asking whether we can combine these notions somehow. In both cases the idea is that you don't necessarily get the exact size you want, but instead the closest size that imagelib can deliver without quality issues (for example without introducing nasty upscaling or changing the aspect ratio).

If we reach consensus that this is the path we want to take, I'd be willing to take care of the necessary imagelib code. As roc said, I think this is fairly easy from an implementation perspective.

Comment 15

[Justin Lebar (not reading bugmail)]

> The image size is an output from the decoder; you can't really modify it, just modify the area into 
> which it's drawn.

I understand.  But what I don't understand is, why is

  <img src="foo.jpg#scalewh=100x100">

better than

  <img src="foo.jpg" width=100 height=100>

Why does the former let you do something that you can't do with the latter?

Note that even if we think media fragments are semantically cleaner for some reason, lots of webpages use <img width height> to rescale images, so if we could make that work better, we'd improve the web for everyone.

Comment 16

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

Well, one issue is that I don't think that approach is applicable everywhere that images can be used. But it'd still be pretty awesome if we could apply that optimization whenever all instances of an image are being drawn at the same size. We'd just have to have a way to get notified when that situation changes and redecode. (Probably we could do that lazily, on Draw.) There are a lot of subleties here but I think this could be made to work.

However, the bottom line is that the media fragments approach is easy, and optimizing the decoded size to the size that will be used for drawing is considerably more complicated. It might be a good idea to take the media fragments approach in the short term, so we can fix problems like the image gallery OOM bug, and move forward with the other approach as a longer-term thing.

Comment 17

[Justin Lebar (not reading bugmail)]

> However, the bottom line is that the media fragments approach is easy

I get this, but I think we should cast a critical eye towards adding duplicative functionality to the web because it's easier for us to implement that way.  The Unix vs. MIT philosophy ship sailed a long time ago for us, and regardless of how we personally feel about worse-is-better, I'd argue that consistency with the existing approach is pretty important.  We shouldn't get to "cheat" just because B2G is having a hard time.

If we could articulate something worthwhile that's doable with image fragments and not with <img height width>, that would change things from my perspective.

Comment 18

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

(In reply to Justin Lebar [:jlebar] from comment #17)
> I get this, but I think we should cast a critical eye towards adding
> duplicative functionality to the web because it's easier for us to implement
> that way.

Agreed. I wouldn't advocate standardizing something like scalewh at this point. (Though I'm not sure it's not a good idea, but I agree that the bar hasn't been met.)

> The Unix vs. MIT philosophy ship sailed a long time ago for us,
> and regardless of how we personally feel about worse-is-better, I'd argue
> that consistency with the existing approach is pretty important.  We
> shouldn't get to "cheat" just because B2G is having a hard time.

Here, I'm not sure. I definitely prefer to do things right the first time, but we shouldn't let an issue that's impacting a large number of B2G users (or just impacting our ability to do good demos or get positive reviews) slide. Do you have a sense of what the consequences will be if this takes months to fix?

Comment 19

[Justin Lebar (not reading bugmail)]

To be clear, there are two separate arguments here.  One is about consistency with the better-is-better design philosophy of the web, and the other is about "cheating" for B2G's sake.

With respect to the latter point, we've been making technical sacrifices for the sake of meeting B2G deadlines for months now.  As far as I can tell we're no closer to shipping than we were many months ago, and we have the downside of increasing amounts of technical debt.

Obviously we can't always do the right thing, but our gut reaction for months now has been "zomg B2G needs to ship next week, we have to hack around it."  I've seen plenty of instances where the hack ends up costing us more than the right fix.

None of the above argues that this particular situation fits into this pattern.  Just that I don't think "can I do this in a week?" is the right design criterion to apply to all B2G-relevant bugs.

The consequence of not fixing this bug immediately is that the gallery app will have difficulty with pictures on the phone that weren't taken on the phone.  Embarrassing though this is, it does not sound like a showstopper to me.

Comment 20

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

That sounds reasonable to me. I don't think it will be too challenging to put in the media fragments solution if the importance of the issue increases later, so we have an escape valve, but for now it sounds like we have the breathing room to focus on the more general optimization instead.

Comment 21

[David Flanagan [:djf]]

(In reply to Justin Lebar [:jlebar] from comment #15)

> 
>   <img src="foo.jpg#scalewh=100x100">
> 
> better than
> 
>   <img src="foo.jpg" width=100 height=100>
> 
> Why does the former let you do something that you can't do with the latter?

If I'm understanding correctly, the first size is a hint: gecko will try to return a high-quality image at about (within a factor of 2?) that size, preserving aspect ratio and avoiding resampling artifacts.

But with the second syntax, the size is mandatory. I know I proposed a JavaScript version of this syntax in comment 0, but it now seems like this would break the web. I'm thinking about things like DOM animations where a site starts off with a image with an artificially small size and progressively enlarges it. We wouldn't want that to require re-decodeing.

If I could rewrite my proposal in comment 0, it would involve making the naturalWidth and naturalHeighth properties writeable and setting those.  I think I like the media fragments solution better than that, however.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Justin Lebar [:jlebar] from comment #15)
> I understand.  But what I don't understand is, why is
> 
>   <img src="foo.jpg#scalewh=100x100">
> 
> better than
> 
>   <img src="foo.jpg" width=100 height=100>
> 
> Why does the former let you do something that you can't do with the latter?

The latter is basically the same as setting style="width:100px; height:100px" to scale the image for layout and drawing, and does not affect the intrinsic size of the image. That means, for example, that if you use this <img> as the parameter of a canvas drawImage() call, the 'width' and 'height' attributes do not affect the results at all, whereas the former would draw the scaled-down image. Likewise the naturalHeight and naturalWidth attributes of the image are unaffected by the latter but would be affected by the former.

These are genuinely different features, and optimizing the latter case to do a scaled-down decode is not easy without affecting cases like canvas usage. If it was, I would have suggested it. I also believe in "doing things right".

(In reply to David Flanagan [:djf] from comment #21)
> If I'm understanding correctly, the first size is a hint: gecko will try to
> return a high-quality image at about (within a factor of 2?) that size,
> preserving aspect ratio and avoiding resampling artifacts.

I wouldn't say it's a hint. It's an instruction to the decoder (and possibly even the network transport, if we had suitable protocol or HTTP extensions) to force the image to exactly the given size modulo aspect ratio preservation. I think it should force the image to an exact size rather than let the UA choose some arbitrary size --- that will be more useful.

Comment 23

[Justin Lebar (not reading bugmail)]

> optimizing the [<img height width>] case to do a scaled-down decode is not easy without affecting 
> cases like canvas usage

It's not clear to me that this is of showstopper difficulty, since we'd have to handle two <img> tags with different width/heights anyway, and that's similar, at least in my crude idea of how this would be implemented.

But you've met my burden of "what additional use-case does media fragments allow" by saying that it lets you draw a downsized image to a canvas, so I relent.

Comment 24

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

re-nom leo? since bug 852061 is leo+ and requires this feature to be implemented.

Comment 25

[Wayne Chang [:wchang]]

Triage 4/12 - Leo+

Comment 26

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> That means, for example, that if you use
> this <img> as the parameter of a canvas drawImage() call, the 'width' and
> 'height' attributes do not affect the results at all, whereas the former
> would draw the scaled-down image.

To be fair, drawImage() can be instructed to draw the image at a specific width and height, and we could lazily decode the image directly to that width and height when drawImage() is actually executed (or at least sometime before the pixels of the updated canvas are needed), so I'm not sure I'm seeing much of a difference here from the <img> tag case.

I definitely agree that this would be very tricky to get right, though, especially in the presence of the same image being used in multiple contexts at different sizes, if we want to "deoptimize" and revert to a shared version of the image at its original size. (The same situation exists in reverse though if we want to "optimize" the media fragment case and have different instances of the image share the same decoded data behind the scenes!)

Comment 27

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: support -moz-scalewh in media fregment

Comment 28

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: downscale image size while decoding JPEG image

NOTE: libjpeg support only 1/2, 1/4, 1/8 image scale while decoding.

Comment 29

[Justin Lebar (not reading bugmail)]

This is a bikeshed, but I'd argue that we should call this scalewh, per our "policy" of not using the -moz prefixes on things we ship in release versions.

Comment 30

[Justin Lebar (not reading bugmail)]

The fact that we don't support scalewh other than 1/2, 1/4, and 1/8 on JPEGs will be visible to websites with this patch.  It will also be visible that we don't support scalewh on anything other than JPEGs.

It seems to me that using the JPEG decoder like this is an internal optimization, and websites shouldn't be able to tell that we're doing so.  If a website provides a scalewh, we must scale the image to that size, one way or another.  We can use the JPEG decoder to optimize this process (perhaps we scale using the JPEG decoder than scale down again ourselves).

We discussed somewhere that we could be clever when doing the scaling ourselves by noticing that we don't have to decode the whole image in order to start rescaling it.  The image decoder gives us one line of the image at a time, and we only need to keep a few lines in memory while rescaling.  This could save memory.  But I think I'd be happy with a patch that doesn't do this.

We also need unit tests for this, please.

Comment 31

[Joe Drew (not getting mail)]

Is the plan to backport media fragments to b2g18? Because without that (and some of the imagelib work Seth did supporting it), getting this supported will be hard.

Comment 32

[David Flanagan [:djf]]

Since the initial use-case for this bug is the FirefoxOS Gallery app, I should point out that that app uses blob: URLs. Do media fragments work with that kind of URL?

Comment 33

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Joe Drew (:JOEDREW! \o/) (Unburying, use needinfo) from comment #31)
> Is the plan to backport media fragments to b2g18? Because without that (and
> some of the imagelib work Seth did supporting it), getting this supported
> will be hard.
We should make this patch work on b2g18 since this bug has been mark as leo+.

Comment 34

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to David Flanagan [:djf] from comment #32)
> Since the initial use-case for this bug is the FirefoxOS Gallery app, I
> should point out that that app uses blob: URLs. Do media fragments work with
> that kind of URL?
Thanks for your reminder, I try to append media fragment on a blob: URL but it's not working. :(

I'm thinking if we can put the image size limitation in pref and let hardware provider decide the value based on the hardware spec?

Comment 35

[Justin Lebar (not reading bugmail)]

We're past the feature-complete deadline for leo.  I think adding media fragments is a feature (and anyway, it's not going to happen in gecko 18).  I also think that placing an arbitrary limit on the size of images that we'll decode is a feature (and a difficult one to support without exposing this hack to the web).

Can we please reconsider this leo+?  Remember that the gallery app works fine with images that were taken with the camera; this bug is only about making the gallery app work with other images.  Note also that this bug will not fix all of our image problems (e.g. bug 860818); this bug only affects very large images.

Comment 36

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

Open for tef? because we're closing bug 847060 which is a tef+, just to verify.

Comment 37

[Justin Lebar (not reading bugmail)]

Please come talk to me in person before tef+'ing this bug.

Comment 38

[u459114]

If we need to fix this issue in 1.0.1, reading upper bound of decode image from pref is safer approach.

Comment 39

[David Flanagan [:djf]]

Milan,

No this isn't the bug that needs to be fixed to resolve the regression caused by 847060.  So unnominating this one.

The bug we need is bug 854799. It is already tef+, and I see now that it actually landed a couple of days ago.

Comment 40

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

Comment on attachment 737098 [details] [diff] [review]
support -moz-scalewh in media fregment

Review of attachment 737098 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/RasterImage.h
@@ +273,5 @@
>    nsIntSize GetRequestedResolution() {
>      return mRequestedResolution;
>    }
>  
> +  /* Provide a hint for the requested dimension of the resulting image. */

A nit, but please use '//'-style comments as that's the style of newer imagelib code.

@@ +274,5 @@
>      return mRequestedResolution;
>    }
>  
> +  /* Provide a hint for the requested dimension of the resulting image. */
> +  void SetRequestedScale(const nsIntSize requestedScale) {

Are these _requested_ scale values? My understanding is that obeying scalewh is _required_. Remove "requested" from everything related to this. mRequestedResolution is called that because it's a hint only and may not be honored. Also, please use Mozilla style for the argument names - i.e., "aRequestedScale" rather than "requestedScale".

@@ +773,5 @@
>  
>    // If the image contains multiple resolutions, a hint as to which one should be used
>    nsIntSize                  mRequestedResolution;
>  
> +  // A hint for image decoder that directly scale the image to smaller buffer

As per my other comment, this isn't a hint, so reword this comment.

::: netwerk/base/src/nsMediaFragmentURIParser.cpp
@@ +380,5 @@
>    }
>  
>    // Parse the media fragment values.
> +  bool gotTemporal = false, gotSpatial = false,
> +      gotResolution = false, gotScale = false;

Nit: please fix the alignment here.

@@ +385,2 @@
>    for (int i = fragments.Length() - 1 ; i >= 0 ; --i) {
>      if (gotTemporal && gotSpatial && gotResolution) {

Add gotScale to this list, or we will ignore scalewh media fragments if we've got one of each of the others.

::: netwerk/base/src/nsMediaFragmentURIParser.h
@@ +67,5 @@
>  
> +  // True if a valid scale media fragment indicated a sampling resolution.
> +  bool HasScale() const { return !mScale.empty(); }
> +
> +  // True if a valid scale media fragment indicated a sampling resolution.

Fix the comment here; it's a copy of the comment for HasScale() instead of matching this method.

Comment 41

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

Comment on attachment 737099 [details] [diff] [review]
downscale image size while decoding JPEG image

Review of attachment 737099 [details] [diff] [review]:
-----------------------------------------------------------------

I concur with Justin's comments: scalewh has to work in every situation. This code or something similar is fine for the JPEG decoder, but you'll need more general support for this elsewhere for other cases. I'd ensure that the work is performed by Decode or RasterImage::DecodeJob in such a way that the rescaling can happen off the main thread. Also, remember to add support for VectorImage (SVG images), which shouldn't be too complex.

We also need to think about how xywh (clipping) and scalewh are supposed to interact. It would be simplest to define scaling as always happening first, which is the direction you've already moved in here. Otherwise the interactions are going to be considerably more complicated.

Comment 42

[Justin Lebar (not reading bugmail)]

Comment on attachment 737098 [details] [diff] [review]
support -moz-scalewh in media fregment

Just to be clear, I do not think this patch is ready to be checked in without major changes.  It sounds like Seth thinks so too.

Comment 43

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Android use SkiaGL to do the image decoding and passing each scan line to a bitmap sampler if scaling is required [1].

We can also create an nsImageSampler to do the sampling job while decoding each scan line, and apply it to all supported image decoder. SkiaGL uses the naive algorithm that only pick the nearest pixel while downscaling. We can improve the image quality by applying low pass filter before scaling [2], but it'll spend more computing power for doing this. Do we need filter+sampling or sampling only?

[1] https://code.google.com/p/skia/source/browse/trunk/src/images/SkScaledBitmapSampler.cpp#424
[2] http://en.wikipedia.org/wiki/Gaussian_blur

Comment 44

[u459114]

SC
I think append filters should be choice-able. For rendering smoothness, we should do this out of rendering thread.

Comment 45

[Joe Drew (not getting mail)]

Unless it's impossible, this should be implemented using our usual Thebes code in the "fallback" case. In the good case, you'll get the closest scaled version using the JPEG decoder, and then scale the rest of the way using gfxContext.

(Note that gfxContext *cannot* be used off the main thread.)

If you want high quality scaling, use mozilla::gfx::Scale, which *can* be used off the main thread.

Comment 46

[David Flanagan [:djf]]

(In reply to Shih-Chiang Chien [:schien] from comment #43)
 Do we need
> filter+sampling or sampling only?

My use case is the Gallery app. Most of the time I'm going to be showing the user a downscaled version of their image and only decoding the whole thing if they ask to zoom in.  So for that case, the downsampled image needs to be high quality or it defeats the purpose.

Comment 47

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

We'd need numbers to be sure which method is better, but for an initial implementation I would be inclined to prefer mozilla::gfx::Scale for the fallback case, based on the above.

Comment 48

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

(In reply to Justin Lebar [:jlebar] from comment #42)
> Just to be clear, I do not think this patch is ready to be checked in
> without major changes.  It sounds like Seth thinks so too.

Absolutely. My r+ does not mean the patch is ready to be checked in. It means I don't need to rereview after you make the needed changes, unless there are substantial changes to the content of the patch.

Comment 49

[Leo]

Need info was set long back...comment #7 . sorry for not updating.

clearing the flag..

Comment 50

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

Let's see if we can revive this bug.

Comment 51

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

My hands are tight for 1.3, feel free to take it if anyone is going to implement this bug.

Comment 52

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

A couple comments here.. scalewh in a media fragment makes total sense.  There are definitely use cases for this, and gallery is a great example.  However, if we want to implement this, we should do it in a generic way that will work for any image format, and that some image formats can optimize (e.g. jpeg for sizes divided by 8).  It should be straightforward to do during image decoding by just scaling down as we go.  It would cause decoding to overall take longer, but memory usage should be significantly down.

We should avoid any implementation where we do a full decode, scale down, and throw away the original decoded image.  That helps long term usage which is good, but it will still cause a memory usage spike that we don't need and that will even exacerbate the problem (by having both the fully decoded image -and- the smaller size image data in memory at the same time).

Comment 53

[David Flanagan [:djf]]

The FirefoxOS Media Team really, really, really needs this feature for the 1.4 release. Until we have it we are stuck with sucky media apps that are prone to out of memory crashes.

I've had some more thoughts about it.  First of all, I've realized that when I'm working with large images where memory usage matters, the first thing I almost always do is copy them to a Canvas.  I don't know if we have a good copy-on-write implementation there for canvas.drawImage(), but I assume that copying to the canvas allocates more memory.

Also, a related problem when working with Image objects is the caching that gecko does. When I'm done with an image, I'd like to release its memory immediately. But gecko holds on to it for a while for reasons that I don't really understand. This seems to happen even for images that are always offscreen.

So instead of implementing the "decode and downsample" feature via media fragments, I'd like to propose that we do it by extending the Canvas API.

Here are two options that seem good to me:

1) modify the 2d drawImage() call so that it can accept an encoded image blob as the first argument (it currently only accepts decoded images in the form of Image, HTMLVideoElement or Canvas). And make the equivalent change to the webgl context as well. The drawImage() arguments allow us to specify any region of the source image to decode and allow us to specify the desired size for it to be decoded at.  Everything we need is already right there in the API.  The issue that probably makes this not work is that it is a synchronous API.

2) So instead, how about we add an asynchronous Canvas.fromBlob() method (to go along with the existing Canvas.toBlob()).  To prevent concurrent modification to the canvas while the decode is happening, I think that this fromBlob() method would have to be a factory method that actually creates and returns a new Canvas element with pixel data initialized from the encoded image blob.

The benefits that I see:

1) by decoding directly into a canvas, I avoid the step of copying from an Image to a canvas

2) by keeping all the image data in a canvas, I assume I get better control over freeing the image when I'm done with it and don't have to deal with unpredictable asynchronous delays before image memory is actually reclaimed.

3) I would imagine that it is easier to implement a single new method like Canvas.fromBlob() than it is to implement new features in an emerging standard like media fragments.

4) It seems easier to ship a feature like Canvas.fromBlob() before standardization than it is to ship new media fragement features.  I gather that we're not moz prefixing stuff anymore, but a single new method would be easier to prefix than a more diffuse set of media fragment features.

Setting needinfo for Vlad to see if this seems like a promising approach.  Also Roc, since he first proposed using media fragments for this.

Comment 54

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think a slightly more spec-friendly way to do this would be to extend createImageBitmap with parameters to scale it to a given size:
http://www.whatwg.org/specs/web-apps/current-work/multipage/timers.html#imagebitmap
probably by creating an overload of createImageBitmap that takes a dictionary of optional parameters.
For various use-cases we already need to extend ImageBitmap with a method (say neuter()) that changes it to a 0x0 image and lets the browser free the underlying memory.

This may be slightly better than decoding into a canvas, which requires extra baggage, not the least of which is that a canvas has to support arbitrary writes, making it harder to share buffers between threads and processes.

One question:
> First of all, I've realized that when I'm working with large images where memory usage matters, the first thing I
> almost always do is copy them to a Canvas.
Why is that?

Comment 55

[David Flanagan [:djf]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #54)
> I think a slightly more spec-friendly way to do this would be to extend
> createImageBitmap with parameters to scale it to a given size:
> http://www.whatwg.org/specs/web-apps/current-work/multipage/timers.
> html#imagebitmap

That sounds great. I haven't been keeping up with the HTML spec and didn't even know about ImageBitmap.  (Is that name already locked in? I'd think that ImagePixels or ImageRaster would make more sense. "Bitmap" implies one bit-per-pixel to me but maybe that's just a throwback to my X11 days.)

> probably by creating an overload of createImageBitmap that takes a
> dictionary of optional parameters.

Would we need any optional parameters other than the desired width and height?  I suppose using a dictionary is the safest option, though.

> For various use-cases we already need to extend ImageBitmap with a method
> (say neuter()) that changes it to a 0x0 image and lets the browser free the
> underlying memory.

Yes, given the amount of memory that could be involved, it seems important not to be forced to rely on garbage collection.

It seems like it would be nice to have an encoder method (like Canvas.toBlob()) defined on ImageBitmap too.  Just for completeness, so we don't have to create an offscreen canvas to do image encoding.

> 
> This may be slightly better than decoding into a canvas, which requires
> extra baggage, not the least of which is that a canvas has to support
> arbitrary writes, making it harder to share buffers between threads and
> processes.
> 

Agreed.

> One question:
> > First of all, I've realized that when I'm working with large images where memory usage matters, the first thing I
> > almost always do is copy them to a Canvas.
> Why is that?

To downsample: to create a thumbnail or a screen-size preview that I can use in the future to avoid decoding the entire image.  I have to do that in the FxOS Gallery, Camera, Video and Music apps. Once I've used an offscreen image and a canvas to produce a smaller size image, I do generally display them with onscreen <img> elements.

Comment 56

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to David Flanagan [:djf] from comment #55)
> That sounds great. I haven't been keeping up with the HTML spec and didn't
> even know about ImageBitmap.  (Is that name already locked in? I'd think
> that ImagePixels or ImageRaster would make more sense. "Bitmap" implies one
> bit-per-pixel to me but maybe that's just a throwback to my X11 days.)

On other platforms, e.g. Win32, a bitmap can have arbitrary depth.

> > One question:
> > > First of all, I've realized that when I'm working with large images where memory usage matters, the first thing I
> > > almost always do is copy them to a Canvas.
> > Why is that?
> 
> To downsample: to create a thumbnail or a screen-size preview that I can use
> in the future to avoid decoding the entire image.  I have to do that in the
> FxOS Gallery, Camera, Video and Music apps. Once I've used an offscreen
> image and a canvas to produce a smaller size image, I do generally display
> them with onscreen <img> elements.

OK thanks. So if you an API to decode straight to a resized ImageBitmap, and to encode from an ImageBitmap, you wouldn't need a canvas.

Comment 57

[Denis Dzyubenko [:ddenis]]

Hi there. I am looking for a (relatively) easy bug to fix to get myself into Mozilla development (my employer is very much interested in FirefoxOS). And stumbled upon this one which seems to be interesting and should directly benefit B2G.

Is there a consensus how this task needs to be solved? As I can see there are two related issues:
1. Gallery app unable to view large images (though relatively minor issue since only externally uploaded images are affected - i.e. not images taken with camera app)
2. Generally high memory consumption on low end devices (Bug 945161)

Though closely related, to me those sounds like different issues that can be solved separately. My assumption is this bug is about improving b2g apps like Gallery and bug 945161 is about generic downscaling of images (<img> tag, canvas) to improve memory consumption on low-end devices.

After reading comments above, a Gallery specific solution was proposed and patch already exists here implementing moz-scalewh media fragment for jpeg. Would it be acceptable to build on top of this patch and add moz-scalewh-based downscaling support to PNG, GIF, BMP and SVG?

Comment 58

[Thinker Li [:sinker]]

Is there any reason that we don't optimize drawImage() by not decoding the image until first drawing |drawImage()|.  Then we know on-screen size to decide whether to do downsampling.  SC, Ting-Yuan and me had discussed this solution offline.  The problem of this solution is blocking the drawImage() call longer for first time.  And, I think it is acceptable for low-end devices as a part of bootstrapping.

For releasing decoded image, we could change image source URI to force a release.

Comment 59

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

(Caveat: I haven't read the previous few comments)

There is no reason why we can't resize on decode, especially with media fragments.  We need to just *do that*.  Coming up with workarounds just layers more workarounds on top of a missing relatively simple feature; let's not complicate things further.

The secondary benefit of decoding directly into a canvas should be treated as a separate issue, though it would be good to be able to do as well.  It's not a full solution (for example, we can't throw away canvas contents as easily as we could images -- there's nothing to redecode from).

Comment 60

[Denis Dzyubenko [:ddenis]]

(In reply to Thinker Li [:sinker] from comment #58)
> Is there any reason that we don't optimize drawImage() by not decoding the
> image until first drawing |drawImage()|.  Then we know on-screen size to
> decide whether to do downsampling.  SC, Ting-Yuan and me had discussed this
> solution offline.  The problem of this solution is blocking the drawImage()
> call longer for first time.  And, I think it is acceptable for low-end
> devices as a part of bootstrapping.

This makes sense and sounds like to good thing to do regardless - no need to decode images that might not even be displayed (e.g. offscreen) and those that are displayed can be downscaled in the first place. The first drawImage() doesn't even need to block if there is a hook to trigger redrawing after the asynch image decoding is complete.

That being said, that sounds like something that should be addressed independently from this issue.

> For releasing decoded image, we could change image source URI to force a
> release.

Comment 61

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

(In reply to Denis Dzyubenko [:ddenis] from comment #60)
> 
> This makes sense and sounds like to good thing to do regardless - no need to
> decode images that might not even be displayed (e.g. offscreen) and those
> ...

Yup, already covered in bug 847223, bug 847221, and most recently enabled on Firefox OS with bug 943248.

Comment 62

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

I'm back to work on this bug for Firefox OS v1.4.

Comment 63

[David Flanagan [:djf]]

Shih-Chiang,

I'm really happy to hear that you have taken this bug!  I'll be filing Gaia bugs that will depend on this one.

Are you going to be doing this with media fragments as discussed previously, or through the ImageBitmap API as Roc proposed in comment #54?

If possible I would prefer a solution that did not use image elements because I think there are still memory management issues for those. Instead, I'd like something (like an ImageBitmap) that gives me a way to discard a decoded image when it is no longer needed and know that the memory will be freed right away.

As noted above, I'd also like a way to decode directly into a canvas, or a way to somehow transfer an ImageBitmap to a Canvas element without copying the memory.  (Roc said in email that this might be something like the transfer functions proposed at http://wiki.whatwg.org/wiki/WorkerCanvas)  (An alternative would be to define a toBlob() function on ImageBitmap so I can convert to a jpeg without copying to a canvas)

One of the most important uses cases (a commited feature for 1.4) is supporting digital zoom in the Camera app.  Right now, we have to decode the full-size image from the camera, copy the desired region to a canvas, and then use canvas.toBlob() on that region. So if the camera is taking 5mp images and the digital zoom has converted that to a 3 mp image, we need 20mb to decode the image plus 12mb for the canvas. On low-end hardware, this is already causing OOM problems.

I'd love a solution to this bug that allows us to do this digital zoom using only 12mb of memory. The main thing is being able to decode just the desired 3mp part of the image. But if I then have to copy that 12mb of data into a canvas in order to convert it to a jpeg, I've double my memory usage, and that is far from ideal.

Comment 64

[David Flanagan [:djf]]

Thinking about this some more, I think I'd like to propose Canvas.fromBlob() as I did in comment #53 again, with arguments to fromBlob() that allow a selected region of the image to be decoded and downsampled to a specified size.  Also the ability to specify maxWidth and maxHeight so we can decode and downsample an entire image without knowing its original size but still maintain its aspect ratio.

Canvas already has a way to release memory (set its size to 0) and there is already a proposal at http://wiki.whatwg.org/wiki/WorkerCanvas to allow canvas memory to be transferred to an ImageBitmap, so it seems simpler to add image decoding to canvas than to modify ImageBitmap to add release and transfer-to-canvas functions.

Comment 65

[Thinker Li [:sinker]]

Hi David, by applying the transform matrix of the context on the real size of the image we get the on-screen size or on-buffer size.  It is not necessary to add or change API for this bug.  For example, the app try to draw an image of 1024x1024 to a canvas with a transform of scaling down to 50%, it just decodes the image to a 512x512 pixels.

It is also true for cropping by using cropping of the context of the canvas.

Comment 66

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

I'm going to take the ImageBitmap API approach for downsampling image decoding and provide a transferToCanvas() for using one smaller buffer on the Gallery app use case.
The digital zoom use case need crop-while-decoding and I think it's an independent feature we can have other people working on it in parallel.
I don't know much about our Canvas implementation so I'm not sure about the effort bringing image decoding to Canvas. @bjacob can you help answering this question?

Comment 67

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to David Flanagan [:djf] from comment #64)
> Canvas already has a way to release memory (set its size to 0) and there is
> already a proposal at http://wiki.whatwg.org/wiki/WorkerCanvas to allow
> canvas memory to be transferred to an ImageBitmap, so it seems simpler to
> add image decoding to canvas than to modify ImageBitmap to add release and
> transfer-to-canvas functions.

See comment #54. Canvas has unnecessary baggage.

(In reply to Thinker Li [:sinker] from comment #65)
> Hi David, by applying the transform matrix of the context on the real size
> of the image we get the on-screen size or on-buffer size.  It is not
> necessary to add or change API for this bug.  For example, the app try to
> draw an image of 1024x1024 to a canvas with a transform of scaling down to
> 50%, it just decodes the image to a 512x512 pixels.

We could do that, if we haven't eagerly or asynchronously decoded the image already. But see above, it's better to avoid canvas in principle.

Comment 68

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

I feel that Jeff Muizelaar would be more knowledgeable than me to answer your question. Jeff: see comment 66.

Comment 69

[David Flanagan [:djf]]

I have sent a proposal for the ImageBitmap changes we're considering to the WHATWG mailing list.  Discussion there might result in ideas we can use here.

The thread begins with this message from me: http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2013-December/041802.html

Comment 71

[u459114]

Memory shrink issue and target to be fixed in 1.4. 
Assign to SC before we have a clear plan on 
1. Who will do it.
2. Make sure the one who works on this issue can commit schedule.

What SC is working in this bug is ImageBitmap approach.

Comment 72

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

Just to update - We've asked SC to get us the design for the proposed solution so that we can have imagelib owners/peers review it.  Once we have the design, we will try to split it so that multiple people can work on this and accelerate the delivery.  Ali, hopefully you'll be available for part of that work.

Comment 73

[Joe Cheng [:jcheng] (please needinfo)]

hi SC, do you mind providing some updates on this? thanks

Comment 74

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Downsampling-while-decoding can be divided into two parts:
1. With given destination image height and width, how do we decode the image directly onto the smaller buffer?
  All types of image decoder in Gecko are output pixel by scan line. The simplest way is to fill each pixel on destination buffer with the nearest point on from the output scan line. The peak memory usage should be the downsized image buffer plus the image buffer for one scan line.

2. How to obtain the destination image height and with before image decoding?
  Gecko already support decode-on-draw. We probably can get the destination size from layout and canvas, or we can introduce new arguments on ImageBitmap for JS developer.

@kanru is working on bug 945161 which can share the code for part 1.

Comment 75

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

Thanks Shih-Chiang - that gives a direction on the low level approach.  There are a number of suggestions and questions that were raised in the comments above (52, 54, 55, etc.), a question of whether the canvas would and should be involved, and a lot of other details to consider.  Are you also putting that information together?

Comment 76

[Ali Ak]

Yep, I'm available. @schien can just give me a ping if he needs help with any subtasks and I'll get in to it. In the meantime I'll carry on with moz2dify pains :)

Comment 77

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

canvas.drawImage() should also get benefit from the downsized image buffer because the copied gfxSurface for drawing is also downsized [1]. Furthermore, we could transfer the image buffer from imgFrame without copy while there is no visible artifact is associated with that imgFrame. Gecko should be able to reconstruct the image buffer if that imgFrame becomes visible again because we already have the discard-on-visibility-change feature. In the usecase of thumbnail generation, we don't need to reconstruct imgFrame because the <img> tag is discard immediately after canvas.drawImage().

[1] http://dxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#4815

Comment 78

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

The overall task is divided into four parts:

1. implement ImageDownsampler, which receives the scanline from decoder and write the downsampled pixel into a downsized image buffer.
2. gather frame size information from layout before full image decoding.
3. modify imgFrame/canvas for using downsized image buffer on drawing.
4. implement the image buffer transferring for imgFrame and use it in CanvasRenderingContext2D::DrawImage()

I'm working on part 1 and part 2. @kanru will take part 3 and bug 945161. Part 4 should be a nice-to-have feature which based on the work of part 1~3. I think we can file a separated bug for it and maybe @Ali can work on it.

Comment 79

[Jeff Muizelaar [:jrmuizel]]

(In reply to Shih-Chiang Chien [:schien] from comment #78)
> The overall task is divided into four parts:
> 
> 1. implement ImageDownsampler, which receives the scanline from decoder and
> write the downsampled pixel into a downsized image buffer.
> 2. gather frame size information from layout before full image decoding.

Can you elaborate what you mean by this? From reading the bug it is still not clear what API is being exposed by this feature.

Comment 80

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #79)
> (In reply to Shih-Chiang Chien [:schien] from comment #78)
> > The overall task is divided into four parts:
> > 
> > 1. implement ImageDownsampler, which receives the scanline from decoder and
> > write the downsampled pixel into a downsized image buffer.
> > 2. gather frame size information from layout before full image decoding.
> 
> Can you elaborate what you mean by this? From reading the bug it is still
> not clear what API is being exposed by this feature.

RasterImage might expose an internal API for updating image size constraint. For the use case of thumbnail generation, we can simply use |<img width="100">| for restricting the buffer size and keep the image aspect ratio.

Comment 81

[Jeff Muizelaar [:jrmuizel]]

(In reply to Shih-Chiang Chien [:schien] from comment #80)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #79)
> > (In reply to Shih-Chiang Chien [:schien] from comment #78)
> > > The overall task is divided into four parts:
> > > 
> > > 1. implement ImageDownsampler, which receives the scanline from decoder and
> > > write the downsampled pixel into a downsized image buffer.
> > > 2. gather frame size information from layout before full image decoding.
> > 
> > Can you elaborate what you mean by this? From reading the bug it is still
> > not clear what API is being exposed by this feature.
> 
> RasterImage might expose an internal API for updating image size constraint.
> For the use case of thumbnail generation, we can simply use |<img
> width="100">| for restricting the buffer size and keep the image aspect
> ratio.

I'm not sure that using width/height is practical. I believe these are animated attributes are animated in a fair number of circumstances (like lightboxes) and we don't want to redecode at the beginning of an animation.

Comment 82

[Ali Ak]

@schien Works for me :) Just add me to the cc list when the bug is added and ill dig in.

Comment 83

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

Let's have Jeff organize this; S.C and Ali, we'll create bugs that block this one and see which ones you can cover.

Comment 84

[Glenn Randers-Pehrson]

libjpeg provides a downscaling capability and it's pretty fast.  We use it in ImageMagick with these calls:

      jpeg_info.scale_num=1U;
      jpeg_info.scale_denom=(unsigned int) scale_factor;
      jpeg_calc_output_dimensions(&jpeg_info);

This is 10-20 times as fast as simply "resizing" a 10-MB JPEG down to 256x256.
If you want to try it, do

      convert -define jpeg:size=512x512 large.jpg -resize 256x256 small.jpg

You can use jpeg:size=256x256 which will be even faster, but I like to do the
more accurate scaling for the final reduction from 512 to 256.

The jpeg_info.scale_* statements could be inserted in image/decoders/nsJPEGDecoder.cpp
just before the jpeg_calc_output_dimensions() that is already present.  We'd of
course need to calculate the scale_factor.

Libpng does not have a similar feature.

Comment 85

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.3T+ it is believed that it's needed for tarako

Comment 86

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

What is the timeframe for 1.3T+?  When is feature complete?

Comment 87

[Keven Kuo [:kkuo]]

About 1.3T timeframe, NI from Steven.

--
Keven

Comment 88

[Joe Cheng [:jcheng] (please needinfo)]

hi Milan, it's March 14th

Comment 89

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

Our goal is to have this in 30.  This means that on March 14 (18th actually), it goes into Aurora, then runs through the Beta and eventually ships on 30 around June 10th.  If this is 1.3T+, somebody can attempt an uplift when we're done, but if done mid March, you will be getting "feature complete" level code, without the benefit of 12 weeks of stabilization, so it may not be appropriate for production.  Anyway, track this bug for progress.

Comment 90

[David Flanagan [:djf]]

(In reply to Joe Cheng [:jcheng] from comment #85)
> triage: 1.3T+ it is believed that it's needed for tarako

Joe: I'm assuming that adding this feature to Gecko won't do any good for Tarako without corresponding patches in Gaia to make use of the feature, right?  So presumably you'll want to create similar blocking bugs for the Gallery app to make use of this feature.  For the gallery app, I expect the work involved to make use of this feature to involve significant refactoring. Depending on when this feature lands in gecko, I would expect to have corresponding gaia features in release 1.5, with perhaps one or two "low hanging fruit" changes making it into 1.4.  Maybe those low-hanging fruit changes can make it into 1.3T, but I have a hard time seeing how we refactor all of Gallery's image handling code for 1.3T.

Milan: I can't really tell from the recent comments if it has been decided what form this feature is going to take.  As a Gaia developer, what will I have to do to decode a large image at a small size?  And will I be able to decode only a specified rectangle of a large image (that would be helpful when the user zooms in on an image).  If we know what the API is going to look like, we can do some planning in advance about how we're going to use it.

Comment 91

[Jeff Muizelaar [:jrmuizel]]

(In reply to David Flanagan [:djf] from comment #90)
> Milan: I can't really tell from the recent comments if it has been decided
> what form this feature is going to take.  As a Gaia developer, what will I
> have to do to decode a large image at a small size?  And will I be able to
> decode only a specified rectangle of a large image (that would be helpful
> when the user zooms in on an image).  If we know what the API is going to
> look like, we can do some planning in advance about how we're going to use
> it.

The current plan is to expose this through media fragments and only for width and height. Only decoding a sub-rect of an image is probably a little more involved. How does that sound?

Comment 92

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

Oh, I really like the idea of a sub-rect! - but let's open a separate bug for that.

Comment 93

[Joe Cheng [:jcheng] (please needinfo)]

1.3T? so it goes back to tarako triage.

Comment 94

[(no longer active)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #91)
> (In reply to David Flanagan [:djf] from comment #90)
> > Milan: I can't really tell from the recent comments if it has been decided
> > what form this feature is going to take.  As a Gaia developer, what will I
> > have to do to decode a large image at a small size?  And will I be able to
> > decode only a specified rectangle of a large image (that would be helpful
> > when the user zooms in on an image).  If we know what the API is going to
> > look like, we can do some planning in advance about how we're going to use
> > it.
> 
> The current plan is to expose this through media fragments and only for
> width and height. Only decoding a sub-rect of an image is probably a little
> more involved. How does that sound?

Also, we should expose the media fragment to certified apps only for now, and gather feedback from the Gaia use cases to see how this idea works in practice, before we feel ready to expose this to the broader web.

Comment 95

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Media fragments feels like a somewhat awkward syntax here. Usually fragments point to a sub-part of a resource that the URL points to. What we have here is more how to display the resource pointed to by the URL.

Something like <img src="..." rescalesize="200x100"> or <img src="..." rescalewidth="200" rescaleheight="100"> or even simply <img src="..." decodetosize> sounds like it'd fit the HTML semantics somewhat better. The first two would decode to the selected size. The last one would always decode to whatever display size layout returns, which would be affected both by CSS and by the normal width/height attributes.

That said, if using URLs makes implementation easier, it's something I could live with too.

Comment 96

[David Flanagan [:djf]]

(In reply to Jonas Sicking (:sicking) from comment #95)

> Something like <img src="..." rescalesize="200x100"> or <img src="..."
> rescalewidth="200" rescaleheight="100"> or even simply <img src="..."
> decodetosize> sounds like it'd fit the HTML semantics somewhat better. The
> first two would decode to the selected size. The last one would always
> decode to whatever display size layout returns, which would be affected both
> by CSS and by the normal width/height attributes.

If you're going to do something like that, let's think about HTML attributes that would be useful when the aspect ratio of the image is not known in advance.  If rescalesize="200x100" meant "preserve aspect ratio and decode the image so that width is 200 or height is 100" that might be nice.

Comment 97

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jonas Sicking (:sicking) from comment #95)
> Media fragments feels like a somewhat awkward syntax here. Usually fragments
> point to a sub-part of a resource that the URL points to. What we have here
> is more how to display the resource pointed to by the URL.
> 
> Something like <img src="..." rescalesize="200x100"> or <img src="..."
> rescalewidth="200" rescaleheight="100"> or even simply <img src="..."
> decodetosize> sounds like it'd fit the HTML semantics somewhat better. The
> first two would decode to the selected size. The last one would always
> decode to whatever display size layout returns, which would be affected both
> by CSS and by the normal width/height attributes.
> 
> That said, if using URLs makes implementation easier, it's something I could
> live with too.

If you do some mental gymnastics you can think of this as choosing half (or some other subset) of the pixels of the image :)

Comment 98

[(no longer active)]

(In reply to comment #95)
> Media fragments feels like a somewhat awkward syntax here. Usually fragments
> point to a sub-part of a resource that the URL points to. What we have here is
> more how to display the resource pointed to by the URL.
> 
> Something like <img src="..." rescalesize="200x100"> or <img src="..."
> rescalewidth="200" rescaleheight="100"> or even simply <img src="..."
> decodetosize> sounds like it'd fit the HTML semantics somewhat better. The
> first two would decode to the selected size. The last one would always decode
> to whatever display size layout returns, which would be affected both by CSS
> and by the normal width/height attributes.
> 
> That said, if using URLs makes implementation easier, it's something I could
> live with too.

I agree that using media fragments here is a bit weird but it has the upside of being usable anywhere a URL to an image is used, which means that it will work for both HTMLImageElement and CSS images.

Comment 99

[Jeff Muizelaar [:jrmuizel]]

Attachment: Rolled up version unbitrotted

Comment 100

[Jeff Muizelaar [:jrmuizel]]

Attachment: Rolled up version w/ the restriction that only certified apps can use it

Comment 101

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The "URLs work everywhere" and "it's kind'a like addressing part of the resource if you squint" are both good arguments. Definitely worth pursuing.

Comment 102

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to David Flanagan [:djf] from comment #96)
> If you're going to do something like that, let's think about HTML attributes
> that would be useful when the aspect ratio of the image is not known in
> advance.  If rescalesize="200x100" meant "preserve aspect ratio and decode
> the image so that width is 200 or height is 100" that might be nice.

I think this is a question we need to ask either way.

Has there been agreement on what features we want to be able to express in the fragment?

Comment 103

[(no longer active)]

(In reply to comment #102)
> (In reply to David Flanagan [:djf] from comment #96)
> > If you're going to do something like that, let's think about HTML attributes
> > that would be useful when the aspect ratio of the image is not known in
> > advance.  If rescalesize="200x100" meant "preserve aspect ratio and decode
> > the image so that width is 200 or height is 100" that might be nice.
> 
> I think this is a question we need to ask either way.
> 
> Has there been agreement on what features we want to be able to express in the
> fragment?

Not entirely.  I think there is general agreement that if we make the media fragment accept sizes across two axes, we don't want the UA to attempt to preserve the aspect ratio, that would be very difficult to use for authors unless they know the original aspect ratio.

I debated whether we want one scale factor or two (one per axis) with Jeff the other day at the office, Jeff preferred the former and I preferred the latter and our discussion didn't go anywhere.

For reference, the existing media fragment for clipping supports specifying a rect with both absolute width and height pixel values and specifying percentages instead of pixel values.

I think there is general agreement that we want to scale first and then clip.

Comment 104

[David Flanagan [:djf]]

Jeff,

I tried to use your rollup patch but it wouldn't compile:

../../../../mozilla-central/image/src/ImageFactory.cpp: In static member function 'static already_AddRefed<mozilla::image::Image> mozilla::image::ImageFactory::CreateRasterImage(nsIRequest*, imgStatusTracker*, const nsCString&, mozilla::image::ImageURL*, uint32_t, uint32_t)':
../../../../mozilla-central/image/src/ImageFactory.cpp:220:9: error: 'nsContentUtils' has not been declared

Comment 105

[Jeff Muizelaar [:jrmuizel]]

You can use this one for now:
https://bugzilla.mozilla.org/attachment.cgi?id=8376452

Comment 106

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

If anybody removes 1.3T, please make sure you leave it as 1.4+.

Comment 107

[Jeff Muizelaar [:jrmuizel]]

Attachment: Switch to -mozsamplefactor

This is a version that I consider landable. I've switched from -moz-scalewh to -moz-samplefactor which takes an integer.

Comment 108

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8380810 [details] [diff] [review]
Switch to -mozsamplefactor

This version is very similar to schien's version. It basically uses -mozsamplefactor instead -mozscalewh. I think the new name makes it more obvious that it might not work with all image formats. This is similar to inSampleSize on Android. (http://developer.android.com/reference/android/graphics/BitmapFactory.Options.html)

Maybe sample size is a better name than sample factor?

Comment 109

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

Let's think real hard before standardizing this. =(

(In reply to Jeff Muizelaar [:jrmuizel] from comment #108)
> Maybe sample size is a better name than sample factor?

It's a bit more intuitive to me, at least.

Comment 110

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

Comment on attachment 8380810 [details] [diff] [review]
Switch to -mozsamplefactor

Review of attachment 8380810 [details] [diff] [review]:
-----------------------------------------------------------------

Solid stuff.

::: netwerk/base/src/nsMediaFragmentURIParser.cpp
@@ +355,5 @@
> +{
> +  int32_t sampleFactor;
> +
> +  // Read and validate coordinates.
> +  if (ParseInteger(aString, sampleFactor) && sampleFactor >= 0) {

0 is legal?

Comment 111

[(no longer active)]

(In reply to comment #109)
> Let's think real hard before standardizing this. =(

I think this will heavily change before we want to even think about standardizing it, but do you have any specific concerns?  I'm really curious to know.

Comment 112

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

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #111)
> (In reply to comment #109)
> > Let's think real hard before standardizing this. =(
> 
> I think this will heavily change before we want to even think about
> standardizing it, but do you have any specific concerns?  I'm really curious
> to know.

My gut feeling is that all these concerns about accommodating situations where the author knows only some subset of {width, height, aspect ratio}, combined with concerns about responsive designs and displays of various DPIs and who knows what else, will inevitably result in either introducing more complexity at the media fragment level than I'm comfortable with, or in a situation where authors manage the complexity in JS instead of letting the platform handle it for them.

I suspect the ideal long term approach is just to infer the optimal size at which to decode the image based upon feedback from the image's consumers (e.g. layout and canvas). This way we can let CSS do the dirty work of specifying complex constraints on image size, and we get the benefits even in situations where content authors haven't optimized for memory savings.

Various commenters have gone back and forth about this issue in this bug multiple times now, I think, and given that these concerns are nothing new I'm definitely not saying that we should hold off on landing -mozsamplefactor. I just think we should continue to aim at a cleaner solution in the long term.

I just recently started looking at making RasterImage support multiple decoded versions of images, which is one of the technical reasons why the cleaner solution was hard to implement up to now, so there is some progress in that direction at least.

Comment 113

[David Flanagan [:djf]]

I've finally tried out the patch in FirefoxOS on a nexus 4.

The good news is that #-moz-samplefactor=n seems to work.

The bad news is that if I omit the media fragment all jpegs default to 1/8th size!  This means the wallpaper is fuzzy and gallery images are badly broken.  Images that are scaled to fit a given size look all pixelated.  And images that are displayed at their natural size are way too small.

Comment 114

[Jeff Muizelaar [:jrmuizel]]

(In reply to David Flanagan [:djf] from comment #113)
> I've finally tried out the patch in FirefoxOS on a nexus 4.
> 
> The good news is that #-moz-samplefactor=n seems to work.
> 
> The bad news is that if I omit the media fragment all jpegs default to 1/8th
> size!  This means the wallpaper is fuzzy and gallery images are badly
> broken.  Images that are scaled to fit a given size look all pixelated.  And
> images that are displayed at their natural size are way too small.

There was an uninitialized value that was probably causing this. The code at
https://tbpl.mozilla.org/?tree=Try&rev=77c3443354c9 should fix this.

Comment 115

[David Flanagan [:djf]]

Thanks. Building the new version now.

I noticed this code in the file that does the fragment parsing:

    if (gotTemporal && gotSpatial && gotResolution) {
       // We've got one of each possible type. No need to look at the rest.

I think you need to add && gotSampleFactor to the list, right?

Also, if you want to change to sample size that is fine with me.  And what's the deal with the -moz- prefix? I thought we weren't using those anymore...

Comment 116

[David Flanagan [:djf]]

Build failed, as you probably already know.  All I see is a bunch of warnings, so I don't know what the error was.

Comment 117

[Jeff Muizelaar [:jrmuizel]]

Attachment: -moz-samplesize

This one should be good.

Comment 118

[Jeff Muizelaar [:jrmuizel]]

Looks like it still has the too small problem.

Comment 119

[Jeff Muizelaar [:jrmuizel]]

I'll figure it out tomorrow.

Comment 120

[David Flanagan [:djf]]

Comment on attachment 8383242 [details] [diff] [review]
-moz-samplesize

Review of attachment 8383242 [details] [diff] [review]:
-----------------------------------------------------------------

I fixed the too small bug by initializing mRequestedSampleFactor() in the RasterImage() constructor. (I wonder if mRequestedResolution also needs initialization there)

::: image/decoders/nsJPEGDecoder.cpp
@@ +240,5 @@
>               ("} (JPEG_SUSPENDED)"));
>        return; /* I/O suspension */
>      }
>  
> +    int sampleFactor = mImage.GetRequestedSampleFactor();

I think the bug is that RasterImage::mRequestedSampleFactor is not being initialized, so you're getting random values that are usually > 0 and > 8. 

This fix works for me:

diff --git a/image/src/RasterImage.cpp b/image/src/RasterImage.cpp
--- a/image/src/RasterImage.cpp
+++ b/image/src/RasterImage.cpp
@@ -403,16 +403,17 @@ RasterImage::RasterImage(imgStatusTracke
   mHasSourceData(false),
   mDecoded(false),
   mHasBeenDecoded(false),
   mAnimationFinished(false),
   mFinishing(false),
   mInUpdateImageContainer(false),
   mWantFullDecode(false),
   mPendingError(false),
+  mRequestedSampleFactor(0),
   mScaleRequest(nullptr)
 {
   mStatusTrackerInit = new imgStatusTrackerInit(this, aStatusTracker);
 
   // Set up the discard tracker node.
   mDiscardTrackerNode.img = this;
   Telemetry::GetHistogramById(Telemetry::IMAGE_DECODE_COUNT)->Add(0);

::: netwerk/base/src/nsMediaFragmentURIParser.cpp
@@ +389,2 @@
>    for (int i = fragments.Length() - 1 ; i >= 0 ; --i) {
>      if (gotTemporal && gotSpatial && gotResolution) {

add && gotSampleFactor to this line?

::: netwerk/base/src/nsMediaFragmentURIParser.h
@@ +72,5 @@
>  
> +  // True if a valid scale media fragment indicated a sampling resolution.
> +  bool HasSampleFactor() const { return !mSampleFactor.empty(); }
> +
> +  // True if a valid scale media fragment indicated a sampling resolution.

Did you mean to repeat the same comment?

Comment 121

[David Flanagan [:djf]]

Jeff,

In comment #117, you said "-moz-samplesize", but the patch you attached there still uses -moz-samplefactor.
Intentional?

Comment 122

[David Flanagan [:djf]]

Attachment: 5mp photo, decoded full size and then downsampled to 600x800 device pixels

This is a photo decoded normally and displayed at 300x400 CSS pixels. Its on a nexus4, so 600x800 device pixels

Comment 123

[David Flanagan [:djf]]

Attachment: same photo, decoded with -moz-samplefactor=3

This is the same photo decoded with #-moz-samplefactor=3 and displayed at 600x800.

Note the subtle differences in the way the mesh fabric of the bear's skirt looks.  I think that this version looks better. The full-size one seems like it might have sampling artifacts in that mesh.

Patryk: you have a trained eye for this sort of thing. Do you think that the second image looks at least as good as the first?  It is using 1/4 of the memory of the first so we want to switch if there is no visual degradation.

Comment 124

[Patryk Adamczyk [:patryk] UX]

David, I tried these on my inari and Nexus 5. Overall there is much less difference between the two on the lower spec'd device. Yes it appears that the sampler is over sharpening the image. But if this means that it allows us to preview the image, then its an okay compromise, especially if this is just a preview and the original image doesn't get affected.

So my suggestion would be to lower the sharpening if we can. If we can't, then this is acceptable.
Thanks for looping me in.

Comment 125

[David Flanagan [:djf]]

Patryk,

Wait, which one seems over sharpened?  The one that say "photo decoded full size". That is just normal unpatched gecko, what we get now.  The one that says "photo sampled and decoded" is the one that is produced through a new process.  Does it appear over-sharpened to you?

Comment 126

[Jeff Muizelaar [:jrmuizel]]

(In reply to David Flanagan [:djf] from comment #120)
> Comment on attachment 8383242 [details] [diff] [review]
> -moz-samplesize
> 
> Review of attachment 8383242 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I fixed the too small bug by initializing mRequestedSampleFactor() in the
> RasterImage() constructor. (I wonder if mRequestedResolution also needs
> initialization there)
> 

Indeed I unintentionally pushed an old version to try. This one was what I meant to push. https://tbpl.mozilla.org/?tree=Try&rev=2c594a196f35

Comment 127

[Patryk Adamczyk [:patryk] UX]

Wow. My bad.
https://bug854795.bugzilla.mozilla.org/attachment.cgi?id=8383525 looks over sharpened.
https://bug854795.bugzilla.mozilla.org/attachment.cgi?id=8383526 looks good.
I see that 8383526 uses the new sampler from comment 123. So I think we're good to go. I for some reason I thought attachment 8383526 [details] was the original, so it fooled me!

Comment 128

[David Flanagan [:djf]]

Attachment: Gaia UITest panel to demonstrate -moz-samplefactor

This is a pull request to add a -moz-samplesize panel to the UITests app.

Russ: if you're able to build gecko with the patch here, would you review this Gaia patch, please?

Jeff: If you'd rather that I open a new bug for this gaia piece, I can do that and attach this patch there.

Comment 129

[David Flanagan [:djf]]

Jeff,

One of the features of the test patch I just attached is that it allows me to display multiple copies of an image either decoded full size or decoded with -moz-samplesize=8.

I was using 4 copies of a 5mp image.  I would expect that decoding them full-size would take 80mb.  And that decoding them at samplesize 8 would require 80/64 mb.

I get_about_memory.py isn't working for my nexus4, so I'm limited to the memory reports I can get with adb shell b2g-ps.

What I find is that displaying the full size image bumps the process memory up by about 40mb: less than I expected.  And displaying the sample images bumps the process memory up by about 10mb, or much more than I expected.

So there is an obvious memory savings here, but it is not as much as I expected.  Could it be that the samplesize 8 is only saving me 8x memory instead of 64x?

Or it could be that my tools are just too blunt to analyze memory usage here.  Have you investigated to confirm that we're saving as much memory as we hope to?

Comment 130

[David Flanagan [:djf]]

Needinfo for Mike in case anyone on the performance team wants to start playing with this feature.

Comment 131

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/86d91f902667

Comment 132

[Nicholas Nethercote [inactive]]

> get_about_memory.py isn't working for my nexus4

What's the problem? Are you running |MOZ_IGNORE_NUWA_PROCESS=1 tools/get_about_memory.py|? That's necessary at the moment.

Comment 133

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Nicholas Nethercote [:njn] from comment #132)
> > get_about_memory.py isn't working for my nexus4
> 
> What's the problem? Are you running |MOZ_IGNORE_NUWA_PROCESS=1
> tools/get_about_memory.py|? That's necessary at the moment.

We should really change the script to do something like `adb shell b2g-ps | grep Nuwa` and set that variable automatically.

Comment 134

[Joe Cheng [:jcheng] (please needinfo)]

traige: 1.3T+ to get this into tarako

Comment 135

[Russ Nicoletti [:russn]]

Comment on attachment 8383959 [details] [review]
Gaia UITest panel to demonstrate -moz-samplefactor

Very nice looking test. I was able to verify the new test works with the this gecko build: https://hg.mozilla.org/mozilla-central/rev/529b86b92b1d.

I also confirmed memory savings when loading an image using moz-samplesize. When loading a 1526x2048 image with no sampling, get_about_memory.py reports UI tests images consume 25.61 MB. When loading the same image using -moz-samplesize=8, get_about_memory.py reports UI tests images consume 2.79 MB. I'm not sure about these numbers but it seems appropriate that the when loading using -moz-samplesize=8 the memory consumed is approximately 8x less than using no sampling. 

I am giving r- because even though most of my review comments are nits, there is a duplication issue that I think should be taken care of.

Comment 136

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/689f7a0c424d