996455 - [Midori][Gallery]View pictures don't fluency

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Gallery, defect, P2)

Description

[sync-1]

Mozilla build ID:20140404164003
 
 DEFECT DESCRIPTION:
 View pictures don't fluency
 
  REPRODUCING PROCEDURES:
 Pre:There are some pictures on Gallery
 1.Enter Gallery,click a picture to view-->view pictures don't fluency-->KO1
 2.Slide left to view next pictures also not fluency-->KO2
 
 show first half picture at first then show whole picture  
 
  EXPECTED BEHAVIOUR:
 KO：View pictures is fluency
 
  USER IMPACT:
 Mid
 
  REPRODUCING RATE:
 5/5
 
  For FT PR, Please list reference mobile's behavior:
 Beetle Lite FF V18H+NA is ok
 Soul 3.5 FF V121 is ok

Comment 1

[sync-1]

Attachment: Soul 3.5 FF预览照片视频.rar

Comment 2

[sync-1]

Attachment: Miata 3G预览照片视频.rar

Comment 3

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

This is a cert blocker

Comment 4

[Jason Smith [:jsmith]]

Vance - Isn't this a vendor issue? The reporter says this works fine on Sora, which makes me think this is device-specific.

Comment 5

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

(In reply to Jason Smith [:jsmith] from comment #4)
> Vance - Isn't this a vendor issue? The reporter says this works fine on
> Sora, which makes me think this is device-specific.

I am thinking, because the camera hardware is different(Sora is 2MP, Midori is 5MP), this issue might be a performance issue(the performance is bad when handling the high resolution pictures.)

Maybe we can test on Flame 1.3 to see if we can reproduce this issue as well since Flame is also come with 5MP camera

Comment 6

[Jason Smith [:jsmith]]

QA Wanted to test this on Flame to see if we can reproduce.

Comment 7

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

(In reply to Jason Smith [:jsmith] from comment #6)
> QA Wanted to test this on Flame to see if we can reproduce.

On second thought, we should verify this issue with Peak since it has high resolution camera and also use the same qHD display as Midori

Comment 8

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

I have 22 pictures in Gallery, all taken by Flame.

Viewing pictures in Gallery seems smooth and everything works as expected on Flame with just base image v10G-2.

I did notice the transition between pictures could be a little bit sluggish, not completely smooth, when holding the phone horizontally/landscape. The issue is minor. I'm not sure if it's related to this bug since I don't see a bug reproducing video (attachments from comment 1 and comment 2 don't seem to be reproducing the bug), and I doubt the issue that I'm seeing could be well captured in video either.

Comment 9

[Jason Smith [:jsmith]]

(In reply to Vance Chen [:vchen][[e-mail]] from comment #7)
> (In reply to Jason Smith [:jsmith] from comment #6)
> > QA Wanted to test this on Flame to see if we can reproduce.
> 
> On second thought, we should verify this issue with Peak since it has high
> resolution camera and also use the same qHD display as Midori

QA doesn't have Peak devices.

Hema - Does anyone on your team have a Peak?

Comment 10

[David Flanagan [:djf]]

The issue is probably that the photos don't have big enough EXIF previews in them, so we have to decode.

If the camera produces photos where the EXIF preview is not bigger that the screen (in device pixels, not css pixels) then previewing those photos is less efficient.

If someone could attach sample photos we could probably test this on any device and wouldn't need to find a peak.

Please attach sample photos that demonstrate the problem.

Comment 11

[David Flanagan [:djf]]

Also, please attach a logcat

Comment 12

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

Attachment: pictures.zip

Attached pictures

Comment 13

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

(In reply to David Flanagan [:djf] from comment #11)
> Also, please attach a logcat

Just attach some photos that demonstrate the problem, will attach the logcat later

Thanks

Comment 14

[Hema Koka [:hema]]

Gallery guys in the team do not have a peak. But lets first investigate with sample pics and logcat info and see if this is happening on devices that we have.

Comment 15

[Hema Koka [:hema]]

David,

Could you please try reproducing this with attached sample photos?

Comment 16

[David Flanagan [:djf]]

I can't reproduce this with a 1.3 build on a Hamachi or Flame using the attached photos. (Though the 1.3 build on the Flame is an old one).

If the issue is, in fact, that the EXIF previews in the photos are not big enough for a qHD display, then what should be happening is that during the scanning process the Gallery app will create preview images in a hidden ".gallery/" directory of the sd card.  If that is failing, for some reason, then when the user tries to preview an image, the app will have to decode the full-size photograph rather than the smaller EXIF preview or the preview file in .gallery/.  

This could be failing is someone modified the gallery app's manifest to remove write permission.  Or if there is something about hte underlying filesystem on the affected device that doesn't allow directories with '.' as the first character, I suppose.

This could also be failing if the vendor attempted to uplift some of the recent 1.3T patches into the 1.3 tree.  There have been a number of changes in 1.3T that replace the .gallery/ preview with the use of the #-moz-samplesize media fragment instead. There are lots of great peformance gains there, and it would be very tempting to try those patches out in 1.3. The problem is that the necessary gecko patches haven't landed in 1.3, so uplifting the gaia patches will just break things.

I can't reproduce, but assuming that you can reproduce this Vance, two things that would be helpful are to know if you have a .gallery/ directory on your phone and whether it contains preview images for the photos that you're having problems with.

Also, if you send me the contents of /system/b2g/webapps/gallery.gaiamobile.org/ from your phone, I can check to see if there have been gaia changes made that are not in our 1.3 tree.

Finally, we get best scanning and preview performance from the gallery app when the camera can produce EXIF previews that are as big as the screen. If this is not happening on the Midori, the fallback, as described above is to manually create previews and store them in the .gallery/ directory of the sdcard.  If, however, the camera can produce previews that are almost big enough, we can set variables in the camera.json file to that will tell camera to use the EXIF preview even though it does not quite fill the screen.  I haven't actually checked what the size of the EXIF previews is in these sample photos, but if they are almost qHD size, the you might encourage the vendor to set the "requiredEXIFPreviewSize" property in camera.json, following the directions in apps/camera/js/config.js

Comment 17

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

Attachment: viewpicture.txt

Hi David -

Actually this is not about preview, this issue happens when you click one picture from the grid view, and then start to swipe to next image, you will then see the phone decode the top-half of the image, then the bottom part. A video for the issue can be found here:

https://www.youtube.com/watch?v=h-zx7aVk3xw

Also attached please find the logcat you asked for

Comment 18

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

Attachment: gallery-gaiamobile-org.zip

The contents of /system/b2g/webapps/gallery.gaiamobile.org/

Comment 19

[David Flanagan [:djf]]

Neither the logcat nor the zip file show anything obvious.  I'll tweak the 1.3 gallery app so it thinks my Flame has a qHD display and see if I can reproduce it that way.

Comment 20

[David Flanagan [:djf]]

Okay, I think I understand what is going on.  The preview images you've sent have 640x480 previews embedded in them.  These are big enough for the 480x854 screen of a Flame, but are not big enough for the 960x540 qHD screen of the Midori. There are two places in the gallery app that check this. The bug is that in MetadataParser.js (when we're scanning) we use the screen size in CSS pixels rather than device pixels.  So when we scan the photos we think the preview size is big enough and we don't generate an external preview image.

But then when it comes time to display the image, we look again at the preview size and compare it to the screen size in device pixels and decide it is not big enough. Since there isn't an external preview image created, we display the full-size image.  Since it is a pretty big image this takes a little time, and you see the lack of image display "fluency" described in the bug report.

I will attach a patch that modifies MetadataParser.js to use device pixels instead of CSS pixels. Vance, can you verify that this fixes the bug on the Midori, please?

Note that the side effect of applying this patch is that the gallery app will have to create its external .gallery/ directory to store image previews in for every photo taken by the camera. And in order to create those image previews the app has to decode the full-size image of every photo. It will cause a significant slow down in the time it takes to scan new photos when the gallery starts up.

We should probably land this patch no matter what, but in addition, please consider the following two options to avoid the scanning performance issue:

1) If the vendor can modify their camera firmware so that the camera can generate 540x720 previews instead of 480x640, then the preview is big enough and the problem goes away.

Another possible fix is to modify the camera.json file to force the gallery app to accept the existing 480x640 previews as good enough.  This will mean that everything will just work, but the 480x640 preview will be scaled up a bit to fit the screen and won't be as sharp as possible. This will give the best performance, at the cost of some visual sharpness.  If the user double taps or does a pinch gesture, then we'll zoom in and load the full image, so the sharpness issue will go away in that case. There are instructions for making the change to camera.js in apps/gallery/js/config.js:

  //
  // Optionally, you can also define variables to specify the
  // minimum EXIF preview size that will be displayed as a
  // full-screen preview by adding a property like this:
  //
  // "requiredEXIFPreviewSize": { "width": 640, "height": 480}
  //
  // If you do not specify this property then EXIF previews will only
  // be used if they are big enough to fill the screen in either
  // width or height in both landscape and portrait mode.
  //

Comment 21

[David Flanagan [:djf]]

Attachment: link to patch on github

Punam: this is just a two line patch for a 1.3 blocking bug. Would you review, please?

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]

[Bug caused by] (feature/regressing bug #): 996455

[User impact] if declined: slow image display in gallery, cert blocker

[Testing completed]: no, I don't have a device that reproduces the bug

[Risk to taking this patch] (and alternatives if risky): this patch can only affect hidpi devices that have devicePixelRatio > 1. It might decrease scanning performance, but will improve image preview performance which is the more important one. If it causes undesired changes for other devices, the camera.json build-time config file allows the change to be bypassed.

[String changes made]: none

Comment 22

[congqingbin]

hi David,

there still have the same problem ,after i merge the patch.

Comment 23

[ying.xu]

(In reply to Vance Chen [:vchen][[e-mail]] from comment #17)
> Hi David -
> 
> Actually this is not about preview, this issue happens when you click one
> picture from the grid view, and then start to swipe to next image, you will
> then see the phone decode the top-half of the image, then the bottom part. A
> video for the issue can be found here:
> 
> https://www.youtube.com/watch?v=h-zx7aVk3xw

Unfortunately , tarako with 1.3t branch has the same problem
the season is as David talked in Comment 10  
"The issue is probably that the photos don't have big enough EXIF previews in them, so we have to decode"

Comment 24

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

(In reply to ying.xu from comment #23)
> (In reply to Vance Chen [:vchen][[e-mail]] from comment #17)
> > Hi David -
> > 
> > Actually this is not about preview, this issue happens when you click one
> > picture from the grid view, and then start to swipe to next image, you will
> > then see the phone decode the top-half of the image, then the bottom part. A
> > video for the issue can be found here:
> > 
> > https://www.youtube.com/watch?v=h-zx7aVk3xw
> 
> Unfortunately , tarako with 1.3t branch has the same problem
> the season is as David talked in Comment 10  
> "The issue is probably that the photos don't have big enough EXIF previews
> in them, so we have to decode"

Hi Ying Xu, have you tried the patch? any luck with that?

Thanks

Comment 25

[ying.xu]

(In reply to David Flanagan [:djf] from comment #16)

> If the issue is, in fact, that the EXIF previews in the photos are not big
> enough for a qHD display, then what should be happening is that during the
> scanning process the Gallery app will create preview images in a hidden
> ".gallery/" directory of the sd card.

This not happened on 1.3t branch

Comment 26

[ying.xu]

(In reply to Vance Chen [:vchen][[e-mail]] from comment #24)
> (In reply to ying.xu from comment #23)
> Hi Ying Xu, have you tried the patch? any luck with that?
> 
> Thanks

The code with 1.3t branch is as below. I think it contained the patch in Comment 21.

  // The screen size. Preview images must be at least this big
  // Note that we use screen.width instead of window.innerWidth because
  // the window size is different when we are invoked for a pick activity
  // (non-fullscreen) and when we are invoked normally (fullscreen).
  var sw = screen.width * window.devicePixelRatio;
  var sh = screen.height * window.devicePixelRatio;

gaia commit id
commit fe4b20403e6fee8b54a69aa872bfb35cbc9af651

Comment 27

[ying.xu]

(In reply to David Flanagan [:djf] from comment #20)
 
> 1) If the vendor can modify their camera firmware so that the camera can
> generate 540x720 previews instead of 480x640, then the preview is big enough
> and the problem goes away.

this will take more memory(continues physical memory) when taking a photo in camera app.
which is a bit hard for tarako.

Comment 28

[ying.xu]

(In reply to ying.xu from comment #25)
> (In reply to David Flanagan [:djf] from comment #16)
> 
> > If the issue is, in fact, that the EXIF previews in the photos are not big
> > enough for a qHD display, then what should be happening is that during the
> > scanning process the Gallery app will create preview images in a hidden
> > ".gallery/" directory of the sd card.
> 
> This not happened on 1.3t branch

We really need this feature.
We can save the preview files when viewing a photo first time.
When we view the photo again, we can use the preview file instead of the original file.

Comment 29

[David Flanagan [:djf]]

(In reply to congqingbin from comment #22)
> hi David,
> 
> there still have the same problem ,after i merge the patch.

The patch will only work for new photos, or if you delete the database and force the gallery app to rescan the existing photos.   Please try again with new photos, and see the bug goes away. Also, it would be helpful to know whether you see preview images being created in a .gallery/ directory of the sdcard.

Vance: what are your findings: does this patch seem to fix the bug for you?

Comment 30

[Punam Dahiya [:pdahiya]]

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

Patch looks good and should be landed on 1.3. 

This fix is already in 1.3T and i am not able to replicate the issue (shown in video #comment 17) on tarako device.

Comment 31

[David Flanagan [:djf]]

(In reply to ying.xu from comment #23)
> 
> Unfortunately , tarako with 1.3t branch has the same problem
> the season is as David talked in Comment 10  
> "The issue is probably that the photos don't have big enough EXIF previews
> in them, so we have to decode"

James and Ying,

This bug is a 1.3 certification blocker for TCL. Let's keep it focused on that one branch.

The 1.3T branch is completely different and would require a completely different fix. If you really think this needs to be fixed for Tarako, please file a new bug and nominate it. (On 1.3T, we can use #-moz-samplesize to decode images at smaller sizes, so we don't save an external preview image to /sdcard/.gallery/ for every photo. Instead we just display the full-size photo with #-moz-samplesize.  This is slightly slower at display time but the speed seems quite good to me considering the hardware. Doing it this way improves scanning time and reduces usage of the sdcard.  If you really don't like the current behavior, you could also try the camera.json customization described above.)  In any case let's have the 1.3T discussion in a different bug.

Comment 32

[ying.xu]

(In reply to David Flanagan [:djf] from comment #31)
> (In reply to ying.xu from comment #23)
>  In any case let's have the 1.3T discussion in a different
> bug.

OK

Comment 33

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

I cannot reproduce this issue with David's patch, also after discussing with TCL, they confirmed that this issue is solved with the patch too. So please help to land this one onto 1.3

David, thanks for your help!

Comment 34

[James Zhang (Spreadtrum)]

Fixed on my side by modified camera thumbnail size.


commit 44d86db264b0025c88c76ada7c8cf59e3eb0d9f8
Author: james.zhang <[e-mail]>
Date:   Thu Jun 5 16:07:18 2014 +0800

    Bug #308512 modify camera thumbnail size to improve gallery preview performance
    
    [bug number  ]
    [root cause  ]
    [changes     ]
    [side effects]
    [self test   ] yes
    [reviewers   ]
    
    Change-Id: I3b73928f95737d2c86ee40c687458edb5bd6d780

Comment 35

[James Zhang (Spreadtrum)]

commit 44d86db264b0025c88c76ada7c8cf59e3eb0d9f8
Author: james.zhang <[e-mail]>
Date:   Thu Jun 5 16:07:18 2014 +0800

    Bug #308512 modify camera thumbnail size to improve gallery preview performance
    
    [bug number  ]
    [root cause  ]
    [changes     ]
    [side effects]
    [self test   ] yes
    [reviewers   ]
    
    Change-Id: I3b73928f95737d2c86ee40c687458edb5bd6d780

diff --git a/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h b/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h
index 6704bc0..42a6325 100755
--- a/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h
+++ b/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h
@@ -375,10 +375,10 @@ struct config_element sprd_back_camera_hardware_config[] = {
        {"preview-frame-rate", "20"},
        {"preview-fps-range-values", "(10000,30000)"},
        {"preview-fps-range", "10000,30000"},
-       {"jpeg-thumbnail-size-values", "320x240,0x0"},
-       {"jpeg-thumbnail-width","320"},
-       {"jpeg-thumbnail-height", "240"},
-       {"jpeg-thumbnail-quality", "80"},
+       {"jpeg-thumbnail-size-values", "640x480,0x0"},
+       {"jpeg-thumbnail-width","640"},
+       {"jpeg-thumbnail-height", "480"},
+       {"jpeg-thumbnail-quality", "70"},
        {"effect-values",

Comment 36

[James Zhang (Spreadtrum)]

We also fix this issue on v1.4 dolphin.

Comment 37

[David Flanagan [:djf]]

Landed to v1.3: https://github.com/mozilla-b2g/gaia/commit/064099549e6e187332fecb130c28d114675139cb

Comment 38

[David Flanagan [:djf]]

(In reply to James Zhang from comment #35)

> +       {"jpeg-thumbnail-width","640"},
> +       {"jpeg-thumbnail-height", "480"},
> +       {"jpeg-thumbnail-quality", "70"},
>         {"effect-values",

480x360 would be better, if your hardware can do it.  In 1.3T, when we take a picture via a pick activity (from the MMS app, for example) the picture size is 640x480, so it is kind of funny that we're embedding a 640x480 preview in that. Often, though, we're rotating the picture and that strips out the EXIF preview.

Comment 39

[Dafeng Xu]

The hardware can do it. but if we use 480x360, the speed of taking picture become slowly, you can change the size in device/sprd/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h

Comment 40

[James Zhang (Spreadtrum)]

(In reply to David Flanagan [:djf] from comment #38)
> (In reply to James Zhang from comment #35)
> 
> > +       {"jpeg-thumbnail-width","640"},
> > +       {"jpeg-thumbnail-height", "480"},
> > +       {"jpeg-thumbnail-quality", "70"},
> >         {"effect-values",
> 
> 480x360 would be better, if your hardware can do it.  In 1.3T, when we take
> a picture via a pick activity (from the MMS app, for example) the picture
> size is 640x480, so it is kind of funny that we're embedding a 640x480
> preview in that. Often, though, we're rotating the picture and that strips
> out the EXIF preview.

Hi David,

    You can modify the value, then build and flash by yourself. But as Dafeng said, tarako camera is very slow with 480x360 thumbnail size.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

v1.4: https://github.com/mozilla-b2g/gaia/commit/e8493dbd6002d516aa236d67fc74ff31fdd1fca5