979394 - Ogg video thumbnail is corrupted

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Zac C (:zac)]

Attachment: 2014-03-04-17-13-49.png

STR
1. Push this OGG file to the device SD card
https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/resources/VID_0001.ogg
2. Load the Video player app
3. Note the thumbnail


Device:   Hamachi
Gaia      6df4533f14ec2645bb13d8a803a5151583ca13a8
Gecko     https://hg.mozilla.org/mozilla-central/rev/529b86b92b1d
BuildID   20140304040200
Version   30.0a1
ro.build.version.incremental=324
ro.build.date=Thu Dec 19 14:04:55 CST 2013

Sorry I don't have a regression range for this - I only noticed it out of co-incidence when running a test.

Comment 1

[Matthew Vaughan]

The following regression window was found using b2g-inbound builds:

- Last Working -
Device: Buri ENG v1.4 MOZ RIL
BuildID: 20140218064649
Gaia: a5b3110d25510a952417ba98a502250f374ef210
Gecko: 288cb11e26bd
Version: 30.0a1
Firmware Version: V1.2-device.cfg

- First Broken -
Device: Buri ENG v1.4 MOZ RIL
BuildID: 20140218065149
Gaia: a5b3110d25510a952417ba98a502250f374ef210
Gecko: a4a8f80f280c
Version: 30.0a1
Firmware Version: V1.2-device.cfg

The push log for these two builds do not yield any commits for some reason, but here is the URL in case it's useful: http://hg.mozilla.org/releases/mozilla-central/pushloghtml?fromchange=288cb11e26bd&tochange=a4a8f80f280c

Instead, here is the push log for the tinderbox regression window: http://hg.mozilla.org//mozilla-central/pushloghtml?fromchange=267e8340c47a&tochange=6660fdb0885b

Comment 2

[Hema Koka [:hema]]

jim or russ, 

could one of you take a first look at this regression bug? if you pick this for investigation, assign it to yourself.

marking 1.4+ because it is regression issue.

thanks
hema

Comment 3

[Jim Porter (:squib)]

This seems like the most likely commit: http://hg.mozilla.org/mozilla-central/rev/a4a8f80f280c

Comment 4

[Russ Nicoletti [:russn]]

I agree with Jim. The gecko change he referred to in comment# 3 seems the most likely source of the regression.

Comment 5

[Jason Smith [:jsmith]]

(In reply to Jim Porter (:squib) from comment #3)
> This seems like the most likely commit:
> http://hg.mozilla.org/mozilla-central/rev/a4a8f80f280c

Confirmed. Fixed the push log & it points dead center at that bug.

http://hg.mozilla.org/integration/b2g-inbound/pushloghtml?fromchange=288cb11e26bd&tochange=a4a8f80f280c

Comment 6

[Sotaro Ikeda [:sotaro]]

I investigated a little bit. This problem is triggered by Bug 965440 and caused by GrallocImage::DeprecatedGetAsSurface(). It does not handle image format correctly.

Comment 7

[Sotaro Ikeda [:sotaro]]

Change to correct component.

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix YV12 color conversion at GrallocImage

Confirmed the problem fixed on hamachi.

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: patch v2 - Fix YV12 color conversion at GrallocImage

Adjust white space.

Comment 10

[Nicolas Silva [:nical]]

Comment on attachment 8386161 [details] [diff] [review]
patch v2 - Fix YV12 color conversion at GrallocImage

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

::: gfx/layers/GrallocImages.cpp
@@ +265,5 @@
>  
>      return imageSurface.forget();
> +  } else if (format == HAL_PIXEL_FORMAT_YV12) {
> +    gfx::ConvertYCbCrToRGB(mData,
> +                           gfx::SurfaceFormat::R5G6B5,

I'd rather have ImageFormatToSurfaceFormat(imageSurface->GetFormat()) instead of hard coding the format.

@@ +363,5 @@
>      surface->Unmap();
>      return surface;
> +  } else if (format == HAL_PIXEL_FORMAT_YV12) {
> +    gfx::ConvertYCbCrToRGB(mData,
> +                           gfx::SurfaceFormat::R5G6B5,

same remark about hard coding.

Comment 11

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #10)
> Comment on attachment 8386161 [details] [diff] [review]
> patch v2 - Fix YV12 color conversion at GrallocImage
> 
> Review of attachment 8386161 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/GrallocImages.cpp
> @@ +265,5 @@
> >  
> >      return imageSurface.forget();
> > +  } else if (format == HAL_PIXEL_FORMAT_YV12) {
> > +    gfx::ConvertYCbCrToRGB(mData,
> > +                           gfx::SurfaceFormat::R5G6B5,
> 
> I'd rather have ImageFormatToSurfaceFormat(imageSurface->GetFormat())
> instead of hard coding the format.

android::ColorConverter's capability limited to RGB565 conversion. And some code are shared with it. To minimize the change, I hard coded. I might be better to remove the hard coding. I am going to update the path.

Comment 12

[Sotaro Ikeda [:sotaro]]

Attachment: patch v3 - Fix YV12 color conversion at GrallocImage

Apply the comment. Carry "nical.bugzilla: review+".

Comment 13

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=4e43d7a7da56

Comment 14

[Sotaro Ikeda [:sotaro]]

 https://hg.mozilla.org/integration/b2g-inbound/rev/72e574eaf012

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/72e574eaf012

Comment 16

[Jonathan Watt [:jwatt]]

For the record this forgot to add an Unmap() call at one of the return points. I just went ahead and fixed that in bug 980415 (which fixes a bunch of other similar brokenness) just before landing. Please be careful of that in future.

Comment 17

[Sotaro Ikeda [:sotaro]]

Thanks for the notification! I forgot to think about it :-(

Comment 18

[Sotaro Ikeda [:sotaro]]

"Easy to forget to call Unmap" problem seem to be fix by Bug 980233. But it might be better to fix about GrallocImage in this bug.

Comment 19

[Sotaro Ikeda [:sotaro]]

Attachment: patch - fix Unmap

Comment 20

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/b2g-inbound/rev/6acf2bd08ff2

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6acf2bd08ff2