971531 - tarako camera does not generate proper exif previews

Status: RESOLVED FIXED

(Firefox OS Graveyard :: GonkIntegration, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

While using the camera on tarako I saw this in the log:

E/GeckoConsole(11436): Content JS ERROR at app://camera.gaiamobile.org/shared/js/media/media_frame.js:109 in displayImage: The thumbnail contained in the jpeg doesn't fitthe device screen. The full size image is rendered.This might cause out of memory errors

We rely on the camera to provide an exif preview tuned to the device screen size to avoid expensive image manipulation operations in camera and gallery.

We should see what needs to be done to get tarako camera to generate these exif previes.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Ben, do you know if we need to get the OEM to fix the camera EXIF previews or is this something we can do?  Thanks!

Comment 2

[Ben Tian (inactive)]

The max supported jpeg thumbnail size from OEM camera driver (320x240) is smaller than screen size (480x320). ni? James Zhang from OEM.

James, can camera driver increase max supported jpeg thumbnail size to be larger than 480x320? When displaying image, camera app requires jpeg thumbnail size be larger than screen size to save expensive memory usage for upscaling.

Comment 3

[Ben Tian (inactive)]

(In reply to Ben Tian [:btian] from comment #2)
> The max supported jpeg thumbnail size from OEM camera driver (320x240) is
> smaller than screen size (480x320). ni? James Zhang from OEM.
> 
> James, can camera driver increase max supported jpeg thumbnail size to be
> larger than 480x320? When displaying image, camera app requires jpeg
> thumbnail size be larger than screen size to save expensive memory usage for
> upscaling.
Note camera app requires only one dimension of jpeg thumbnail size be larger than screen size according to [1].

[1] https://github.com/mozilla-b2g/gaia/blob/v1.3t/shared/js/media/media_frame.js#L95

Comment 4

[ying.xu]

Please apply this patch for a temporary fix.
Not test yet, I am testing it.

diff --git a/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h b/common/libs/libcamera/sc8810/SprdCameraHardwareC
index b781783..9ce826f 100755
--- a/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h
+++ b/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h
@@ -373,7 +373,7 @@ struct config_element sprd_back_camera_hardware_config[] = {
        {"preview-fps-range", "10000,30000"},
        {"jpeg-thumbnail-size-values", "320x240,0x0"},
        {"jpeg-thumbnail-width","320"},
-       {"jpeg-thumbnail-height", "240"},
+       {"jpeg-thumbnail-height", "480"},
        {"jpeg-thumbnail-quality", "80"},
        {"effect-values",
 #ifdef CONFIG_CAMERA_788

Comment 5

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

triage: 1.3T+ for release

Comment 6

[ying.xu]

test failed

(In reply to ying.xu from comment #4)
> Please apply this patch for a temporary fix.
> Not test yet, I am testing it.
> 
> diff --git a/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h
> b/common/libs/libcamera/sc8810/SprdCameraHardwareC
> index b781783..9ce826f 100755
> --- a/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h
> +++ b/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h
> @@ -373,7 +373,7 @@ struct config_element sprd_back_camera_hardware_config[]
> = {
>         {"preview-fps-range", "10000,30000"},
>         {"jpeg-thumbnail-size-values", "320x240,0x0"},
>         {"jpeg-thumbnail-width","320"},
> -       {"jpeg-thumbnail-height", "240"},
> +       {"jpeg-thumbnail-height", "480"},
>         {"jpeg-thumbnail-quality", "80"},
>         {"effect-values",
>  #ifdef CONFIG_CAMERA_788

Comment 7

[ying.xu]

please apply this patch ,tested OK

diff --git a/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h b/common/libs/libcamera/sc8810/SprdCameraHardwareC
index b781783..4a4b4f9 100755
--- a/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h
+++ b/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h
@@ -371,9 +371,9 @@ 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-size-values", "320x480,0x0"},
        {"jpeg-thumbnail-width","320"},
-       {"jpeg-thumbnail-height", "240"},
+       {"jpeg-thumbnail-height", "480"},
        {"jpeg-thumbnail-quality", "80"},
        {"effect-values",
 #ifdef CONFIG_CAMERA_788

Comment 8

[Ben Tian (inactive)]

Ying,

The new aspect ratio (2:3) is inconsistent with original one (4:3) and may distort the thumbnail. Have you checked the generated thumbnail? Also how much does jpeg encoder's memory usage increase? We should ensure Tarako allows the additional memory usage.

(In reply to ying.xu from comment #7)
> please apply this patch ,tested OK
> 
> diff --git a/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h
> b/common/libs/libcamera/sc8810/SprdCameraHardwareC
> index b781783..4a4b4f9 100755
> --- a/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h
> +++ b/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h
> @@ -371,9 +371,9 @@ 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-size-values", "320x480,0x0"},
>         {"jpeg-thumbnail-width","320"},
> -       {"jpeg-thumbnail-height", "240"},
> +       {"jpeg-thumbnail-height", "480"},
>         {"jpeg-thumbnail-quality", "80"},
>         {"effect-values",
>  #ifdef CONFIG_CAMERA_788

Comment 9

[ying.xu]

(In reply to Ben Tian [:btian] from comment #8)
> Ying,
> 
> The new aspect ratio (2:3) is inconsistent with original one (4:3) and may
> distort the thumbnail. Have you checked the generated thumbnail?

I didn't check this before. 

> much does jpeg encoder's memory usage increase? We should ensure Tarako
> allows the additional memory usage.

thumbnail generating sequence:

sensor raw data(YUV big) -----> thumbnail data(YUV) ----> thumbnail jpg

Basically, it will consume memory to hold the thumbnail.  That's not too much in contrast with capturing buffer.

Because the ratio problem between the original picture and thumbnail one.

If we keep the ratio the same between them 

we can change thumbnail size bigger, but use more memory.

or we change the size of capture picture , That will involved more modifications(driver, maybe gecko/gaia).

Besides, the pictures with thumbnail that were not taken from camera would have this problem if thumbnail size was smaller than screen size.

So we will not fix this.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Its going to make it difficult for us to have a performant camera app on v1.3 without this bug getting fixed.  Right now we require large amounts of memory to resize images in web contact (like our apps).  Hopefully that will get fixed with bug 854795, but that won't be ready for v1.3.  Our other OEM partners have helped here by generating EXIF thumbnails at least as large as the screen.

David, is that a fair assessment of the situation?

Comment 11

[David Flanagan [:djf]]

Yes, that's a fair assessment.

The latest change is bug 942199 where we've added a build-time configuration option to support a QC device that can't generate previews that are quite big enough.  If we can get a 3:4 thumbnail that is almost big enough (like 300x400), we can use a build-time configuration to treat that as good enough and accept a slight visual degradation in previews in exchange for much improved performance.  So being as big as the screen size is no longer a hard requirement.  But we do need a custom gaia build to relax that size requirement

The EXIF preview does need to match the aspect ratio of the picture. That is a hard requirement.

I don't understand Ying Xu's response in comment #9. Can't the driver support 3:4 thumbnails?  What sizes are available?

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Ying Xu, is it possible to generate a thumbnail in the 3:4 aspect ratio that is almost as big as the screen?  If so, it appears we can compensate with a custom gaia build.

Comment 13

[ying.xu]

(In reply to Ben Kelly [:bkelly] from comment #12)
> Ying Xu, is it possible to generate a thumbnail in the 3:4 aspect ratio that
> is almost as big as the screen?  If so, it appears we can compensate with a
> custom gaia build.

that's a size 360X480, bigger than the whole screen.
we can do it without increasing too much size for thumbnail of 3:2 aspect ratio(720X480)

so you're going use a 360X480 size thumbnail to generate a new one of 3:2 aspect ratio ?

Comment 14

[Ben Tian (inactive)]

(In reply to ying.xu from comment #13)
> that's a size 360X480, bigger than the whole screen.
> we can do it without increasing too much size for thumbnail of 3:2 aspect
> ratio(720X480)
Great. Please provide us a patch to test and let us know once it lands on your codebase.

> so you're going use a 360X480 size thumbnail to generate a new one of 3:2
> aspect ratio ?
Yes. Driver only needs to generate a 480x360 thumbnail that 1) is larger than screen size 480x320 and 2) keeps aspect ratio 4:3. Camera app will fit the 4:3 thumbnail onto 3:2 screen.

Comment 15

[ying.xu]

(In reply to Ben Tian [:btian] from comment #14)
> (In reply to ying.xu from comment #13)
> > that's a size 360X480, bigger than the whole screen.
> > we can do it without increasing too much size for thumbnail of 3:2 aspect
> > ratio(720X480)
> Great. Please provide us a patch to test and let us know once it lands on
> your codebase.

patch

diff --git a/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h b/common/libs/libcamera/sc8810/SprdCameraHardwareC
index b781783..0c290d2 100755
--- a/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h
+++ b/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h
@@ -371,9 +371,9 @@ 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-size-values", "360x480,0x0"},
+       {"jpeg-thumbnail-width","360"},
+       {"jpeg-thumbnail-height", "480"},
        {"jpeg-thumbnail-quality", "80"},
        {"effect-values",
 #ifdef CONFIG_CAMERA_788

when I tried ,I got these log and the thumbnail seem wrong
E/GeckoConsole(  445): Content JS ERROR at app://camera.gaiamobile.org/gaia_build_defer_index.js:87 in Camera.selectThumbnailSize: Error while selecting thumbnail size. There are no thumbnail sizes that match the ratio of the selected picture size: {"width":1600,"height":1200}

Comment 16

[Ben Tian (inactive)]

(In reply to ying.xu from comment #15)
> (In reply to Ben Tian [:btian] from comment #14)
> > (In reply to ying.xu from comment #13)
> > > that's a size 360X480, bigger than the whole screen.
> > > we can do it without increasing too much size for thumbnail of 3:2 aspect
> > > ratio(720X480)
> > Great. Please provide us a patch to test and let us know once it lands on
> > your codebase.
> 
> patch
> 
> diff --git a/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h
> b/common/libs/libcamera/sc8810/SprdCameraHardwareC
> index b781783..0c290d2 100755
> --- a/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h
> +++ b/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h
> @@ -371,9 +371,9 @@ 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-size-values", "360x480,0x0"},
> +       {"jpeg-thumbnail-width","360"},
> +       {"jpeg-thumbnail-height", "480"},
>         {"jpeg-thumbnail-quality", "80"},
>         {"effect-values",
>  #ifdef CONFIG_CAMERA_788
> 
> when I tried ,I got these log and the thumbnail seem wrong
> E/GeckoConsole(  445): Content JS ERROR at
> app://camera.gaiamobile.org/gaia_build_defer_index.js:87 in
> Camera.selectThumbnailSize: Error while selecting thumbnail size. There are
> no thumbnail sizes that match the ratio of the selected picture size:
> {"width":1600,"height":1200}

Please revise the patch as following to match original aspect ratio:

> -       {"jpeg-thumbnail-size-values", "320x240,0x0"},
> -       {"jpeg-thumbnail-width","320"},
> -       {"jpeg-thumbnail-height", "240"},
> +       {"jpeg-thumbnail-size-values", "480x360,0x0"},
> +       {"jpeg-thumbnail-width","480"},
> +       {"jpeg-thumbnail-height", "360"},

Comment 17

[Ben Tian (inactive)]

Ying, does the revision in comment 16 work? Do you meet any other error?

Comment 18

[ying.xu]

(In reply to Ben Tian [:btian] from comment #17)
> Ying, does the revision in comment 16 work? Do you meet any other error?

It works. I see no errors.

Comment 19

[James Zhang (Spreadtrum)]

(In reply to ying.xu from comment #18)
> (In reply to Ben Tian [:btian] from comment #17)
> > Ying, does the revision in comment 16 work? Do you meet any other error?
> 
> It works. I see no errors.

Should be 480x320.

Comment 20

[Ben Tian (inactive)]

(In reply to James Zhang from comment #19)
> (In reply to ying.xu from comment #18)
> > (In reply to Ben Tian [:btian] from comment #17)
> > > Ying, does the revision in comment 16 work? Do you meet any other error?
> > 
> > It works. I see no errors.
> 
> Should be 480x320.

James, what do you mean?
Do you mean screen size or thumbnail size should be 480x320? And the reason?

Comment 21

[James Zhang (Spreadtrum)]

It will cause camera take picture bug, Ying will track it.

Comment 22

[Ben Tian (inactive)]

(In reply to James Zhang from comment #21)
> It will cause camera take picture bug, Ying will track it.
Ying, please explain the bug in detail or let me know the bug number.

Comment 23

[James Ho]

James, could you check if there's any update on your side?

Comment 24

[James Zhang (Spreadtrum)]

In device/sprd

commit 37580d325f6500b65c83b989c906bcc8796d3483
Author: dafeng.xu <[e-mail]>
Date:   Mon Mar 3 16:05:05 2014 +0800

    Bug #281142 the screen reflash is not fast when open picture in camera app
    
    [bug number  ] 281142
    [root cause  ] the thumbnail size is not match the screen size
    [changes     ] change the thumbnail size
    [side effects] none
    [reviewers   ] james.zhang
    
    Change-Id: I2e18c11de24e4d1fb7fe0d2d10fa57f0be19216f

diff --git a/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h b/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h
index b781783..1dff32f 100755
--- a/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h
+++ b/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h
@@ -371,9 +371,9 @@ 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-size-values", "480x320,0x0"},
+       {"jpeg-thumbnail-width","480"},
+       {"jpeg-thumbnail-height", "320"},
        {"jpeg-thumbnail-quality", "80"},
        {"effect-values",
 #ifdef CONFIG_CAMERA_788

Comment 25

[Ben Tian (inactive)]

(In reply to Ben Tian [:btian] from comment #22)
> (In reply to James Zhang from comment #21)
> > It will cause camera take picture bug, Ying will track it.
> Ying, please explain the bug in detail or let me know the bug number.

James/Ying, can you explain the camera bug in detail? The 480x320 (3:2) thumbnail doesn't match picture aspect ratio 4:3 and would result in distorted thumbnail.

Comment 26

[James Zhang (Spreadtrum)]

The thumbnail is gray when the value is 480x360, do you test this value?

Comment 27

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

is it still 480x360 after you revert camera back to 2MP? Thanks

Comment 28

[James Zhang (Spreadtrum)]

(In reply to Joe Cheng [:jcheng] from comment #27)
> is it still 480x360 after you revert camera back to 2MP? Thanks

I mean 480x320 will cause image thumbnail display bug, and we fix this cause by 480x320.
Please find our commit in device/sprd.

commit 37580d325f6500b65c83b989c906bcc8796d3483
Author: dafeng.xu <[e-mail]>
Date:   Mon Mar 3 16:05:05 2014 +0800

    Bug #281142 the screen reflash is not fast when open picture in camera app
    
    [bug number  ] 281142
    [root cause  ] the thumbnail size is not match the screen size
    [changes     ] change the thumbnail size
    [side effects] none
    [reviewers   ] james.zhang
    
    Change-Id: I2e18c11de24e4d1fb7fe0d2d10fa57f0be19216f

diff --git a/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h b/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h
index b781783..1dff32f 100755
--- a/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h
+++ b/common/libs/libcamera/sc8810/SprdCameraHardwareConfig.h
@@ -371,9 +371,9 @@ 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-size-values", "480x320,0x0"},
+       {"jpeg-thumbnail-width","480"},
+       {"jpeg-thumbnail-height", "320"},
        {"jpeg-thumbnail-quality", "80"},
        {"effect-values",
 #ifdef CONFIG_CAMERA_788

Comment 29

[James Zhang (Spreadtrum)]

Sorry, I mean 480x360 has bug, and 480x320 has no bug.
We use 480x320 now.

Comment 30

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

James, do you mean this bug is now fixed on your side? Thanks

Comment 31

[James Zhang (Spreadtrum)]

(In reply to Joe Cheng [:jcheng] from comment #30)
> James, do you mean this bug is now fixed on your side? Thanks

Yes.

Comment 32

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

Thanks James, please resolved fixed the bug when you have fixed it. thank you