Closed Bug 979394 Opened 6 years ago Closed 6 years ago

Ogg video thumbnail is corrupted

Categories

(Core :: Graphics: Layers, defect)

30 Branch
ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
blocking-b2g 1.4+
Tracking Status
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- fixed
b2g-v1.4 --- fixed

People

(Reporter: zcampbell, Assigned: sotaro)

References

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

Attached image 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.
Keywords: regression
Whiteboard: [regression]
blocking-b2g: --- → 1.4?
QA Contact: mvaughan
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
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
blocking-b2g: 1.4? → 1.4+
Flags: needinfo?(squibblyflabbetydoo)
Flags: needinfo?(rnicoletti)
This seems like the most likely commit: http://hg.mozilla.org/mozilla-central/rev/a4a8f80f280c
Flags: needinfo?(squibblyflabbetydoo)
I agree with Jim. The gecko change he referred to in comment# 3 seems the most likely source of the regression.
Flags: needinfo?(rnicoletti)
(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
Blocks: 965440
Component: Gaia::Video → Video/Audio
Product: Firefox OS → Core
Version: unspecified → 30 Branch
Assignee: nobody → sotaro.ikeda.g
I investigated a little bit. This problem is triggered by Bug 965440 and caused by GrallocImage::DeprecatedGetAsSurface(). It does not handle image format correctly.
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Change to correct component.
Component: Video/Audio → Graphics: Layers
Confirmed the problem fixed on hamachi.
Attachment #8386153 - Flags: review?(nical.bugzilla)
Status: NEW → ASSIGNED
Adjust white space.
Attachment #8386153 - Attachment is obsolete: true
Attachment #8386153 - Flags: review?(nical.bugzilla)
Attachment #8386161 - Flags: review?(nical.bugzilla)
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.
Attachment #8386161 - Flags: review?(nical.bugzilla) → review+
(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.
Apply the comment. Carry "nical.bugzilla: review+".
Attachment #8386161 - Attachment is obsolete: true
Attachment #8386856 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/72e574eaf012
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
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.
Thanks for the notification! I forgot to think about it :-(
"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.
Attachment #8387607 - Flags: review?(nical.bugzilla)
Attachment #8387607 - Flags: review?(nical.bugzilla) → review+
You need to log in before you can comment on or make changes to this bug.