Closed Bug 850566 Opened 7 years ago Closed 7 years ago

[Buri][Video]When playing some videos, part of bottom will be stretched a little

Categories

(Firefox OS Graveyard :: General, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:tef+, firefox21 wontfix, firefox22 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

VERIFIED FIXED
B2G C4 (2jan on)
blocking-b2g tef+
Tracking Status
firefox21 --- wontfix
firefox22 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: sync-1, Assigned: ikumar)

Details

Attachments

(7 files, 3 obsolete files)

Firefox v1.0.1
 Mozilla build ID: 20130304070202.
 
 +++ This bug was initially created as a clone of Bug #419477 +++
 
 Created an attachment (id=363966)
 Screen shot
 
 DEFECT DESCRIPTION:
 (1)Play some videos, you will find the bottom part is stretched a little , no matter in portrait mode or Landscape mode.---- NOK
 (2)Samples to check:
 https://www.youtube.com/watch?v=NeGe7lVrXb0&feature=youtube_gdata_player
 https://www.dropbox.com/s/bxgyyrcvyymi4xd/AvatarMovieTrailer.mp4
 
 REPRODUCING PROCEDURES:
 
 EXPECTED BEHAVIOUR:
 
 ASSOCIATE SPECIFICATION:
 
 TEST PLAN REFERENCE:
 
 TOOLS AND PLATFORMS USED:
 
 USER IMPACT:
 big
 
 REPRODUCING RATE:
 100%
 
 For FT PR, Please list reference mobile's behavior:
 
 ++++++++++ end of initial bug #419477 description ++++++++++
 
 
 
 CONTACT INFO (Name,Phone number):
 
 DEFECT DESCRIPTION:
 
 REPRODUCING PROCEDURES:
 
 EXPECTED BEHAVIOUR:
 
 ASSOCIATE SPECIFICATION:
 
 TEST PLAN REFERENCE:
 
 TOOLS AND PLATFORMS USED:
 
 USER IMPACT:
 
 REPRODUCING RATE:
 
 For FT PR, Please list reference mobile's behavior:
Attached image Screen shot
blocking-b2g: --- → tef?
Inder, can you please look at this?
Assignee: nobody → ikumar
blocking-b2g: tef? → tef+
I've seen this before. It seems it's only a problem when Hardware composer is disabled
Does this need to remain a blocker, given comment 3 and our planned config around hardware composer?
blocking-b2g: tef+ → tef?
Flags: needinfo?(mvines)
(ni on dwilson to further qualify comment 3)
Flags: needinfo?(mvines) → needinfo?(dwilson)
diego, I could reproduce this issue on our device even if hw composer is enabled.
(Back to tef+ for now, until Inder is able to triage why these media file are causing trouble.)
blocking-b2g: tef? → tef+
Flags: needinfo?(dwilson)
For AvatarMovieTrailer found that there is a discrepancy between the video dimension in the metadata and what decoder finds. In the metadata the video dimensions are 480x200 whereas the decoder finds it as 480x208. Do we know how and where we discover the video dimension in gecko? I am thinking it gets it from decoder.
CCing some media superstars for comment 8.
There are defect's of handling of SurfaceDescriptorGralloc's image size. SurfaceDescriptorGralloc uses android::GraphicBuffer's Image size as descriptor's image's size. It seems that there are cases that GraphicBuffer's size is different from valid Image size. 

"208" comes from GraphicBuffer's size.
(In reply to Sotaro Ikeda [:sotaro] from comment #10)
> There are defect's of handling of SurfaceDescriptorGralloc's image size.
> SurfaceDescriptorGralloc uses android::GraphicBuffer's Image size as
> descriptor's image's size. It seems that there are cases that
> GraphicBuffer's size is different from valid Image size. 
> 
> "208" comes from GraphicBuffer's size.

I am going to investigate the problem more tomorrow.
I fixed the problem locally. I am going to write a cleaner patch. The problem happens from the size difference between GraphicBuffer's size and actual video size.

When there is the size difference, it is necessary to draw by using LayerManagerOGL::BindAndDrawQuadWithTextureRect(). current implementation use LayerManagerOGL::BindAndDrawQuad().
Comment on attachment 729792 [details] [diff] [review]
Part1 - add image size to SurfaceDescriptorGralloc

:nical, can you review the patch?
Attachment #729792 - Flags: review?(nical.bugzilla)
Comment on attachment 729793 [details] [diff] [review]
Part2 - render gralloc buffer by using image size

:roc, can you review the patch?
Attachment #729793 - Flags: review?(roc)
Comment on attachment 729794 [details] [diff] [review]
Part3 - set video size to SurfaceDescriptorGralloc

:doublec, can you review the patch?
Attachment #729794 - Flags: review?(chris.double)
I confirmed the patches fixed the problem on unagi phone.
Comment on attachment 729794 [details] [diff] [review]
Part3 - set video size to SurfaceDescriptorGralloc

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

::: content/media/omx/OmxDecoder.cpp
@@ +528,5 @@
>        descriptor = mNativeWindow->getSurfaceDescriptorFromBuffer(mVideoBuffer->graphicBuffer().get());
>      }
>  
>      if (descriptor) {
> +      // Change the descriptor's size to video's sizse. There are cases that

Spelling of 'sizse'.
Attachment #729794 - Flags: review?(chris.double) → review+
Comment on attachment 729792 [details] [diff] [review]
Part1 - add image size to SurfaceDescriptorGralloc

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

::: gfx/layers/ipc/LayersSurfaces.ipdlh
@@ +45,5 @@
>  };
>  
>  struct SurfaceDescriptorGralloc {
>    PGrallocBuffer buffer;
> +  nsIntSize size;

Please add a comment here explaining why we store an extra size member instead of just using the android::GraphicBuffer's Image size.
Attachment #729792 - Flags: review?(nical.bugzilla) → review+
Commitable patch. carry "nical.bugzilla: review+".
Attachment #729792 - Attachment is obsolete: true
Attachment #730261 - Flags: review+
Commitable patch. Carry "roc: review+".
Attachment #729793 - Attachment is obsolete: true
Attachment #730262 - Flags: review+
Committable patch. Carry "chris.double: review+".
Attachment #729794 - Attachment is obsolete: true
Attachment #730263 - Flags: review+
Is there a gaia component to this patch?
(In reply to John Ford [:jhford] from comment #28)
> Is there a gaia component to this patch?

IMHO, gaia side patch is not necessary.
Environmental  Variables:
Unagi Build ID: 20130401070203
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/b28463f2e718
Gaia: ddb38ac8a34f9e30e09d0ff3b5c1bfb9b664b7c3

The same issue still reproduces on "Unagi" device.
Comment on attachment 730261 [details] [diff] [review]
Part1 v2 - add image size to SurfaceDescriptorGralloc

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 #): N/A
User impact if declined:Some videos are not rendered correctly 
Testing completed: Tested on Unagui phone.
Risk to taking this patch (and alternatives if risky):  low risk
String or UUID changes made by this patch: none
Attachment #730261 - Flags: approval-mozilla-b2g18?
(In reply to Sotaro Ikeda [:sotaro] from comment #32)
> Comment on attachment 730261 [details] [diff] [review]
> Part1 v2 - add image size to SurfaceDescriptorGralloc
> 
> 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 #): N/A
> User impact if declined:Some videos are not rendered correctly 
> Testing completed: Tested on Unagui phone.
> Risk to taking this patch (and alternatives if risky):  low risk
> String or UUID changes made by this patch: none

This bug is already tef+ so no need for approval to uplift the patches.
Attachment #730261 - Flags: approval-mozilla-b2g18?
Patch for b2g18. Carry "nical.bugzilla: review+"
Patch for b2g18. Carry "roc: review+"
Attachment #734773 - Flags: review+
Patch for b2g18. Carry "chris.double: review+"
Attachment #734774 - Flags: review+
Attachment #734771 - Flags: review+
Attachment #730262 - Attachment description: Part2 - render gralloc buffer by using image size → Part2 v2- render gralloc buffer by using image size
Attachment #734773 - Attachment description: Part2 - render gralloc buffer by using image size → Part2 v2 for b2g18 - render gralloc buffer by using image size
Attachment #734774 - Attachment description: Part3 v2 - set video size to SurfaceDescriptorGralloc → Part3 v2 for b2g18 - set video size to SurfaceDescriptorGralloc
Can I get help landing on gecko b2g18?
Flags: needinfo?(ryanvm)
Flags: needinfo?(ryanvm)
Keywords: checkin-needed
Component: Gaia::Video → General
Working fine in ikura device with:

Gecko  http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/2b44e2c40cc1
Gaia   3e7dda88950ae09166109783adea8181eb857d21
BuildID 20130414230203
Version 18.0
Status: RESOLVED → VERIFIED
Tested on the new release:
AU_LINUX_GECKO_ICS_STRAWBERRY_V1.01.00.01.019.085
Firefox os  v1.0.1
Mozilla build ID:20130422230201

The result is NOK, so I reopen the PR.
The problem is still there, no improvment.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Amelie,

There is a separater patch for hardware composer, which is enabled in that build. Please see bug 865112.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Diego,

So you mean this patch?

https://www.codeaurora.org/gitweb/quic/lf/?p=build.git;a=blob;f=patch/all/gecko/HWC-multirect-visible-region.patch;h=1e1f6bde3a648dceb9d3a7f8bff27054e2030936;hb=refs/heads/v1.1

Are you sure it is uplifted to v1.0.1?
And we can verify the problem in later build?(after 4/25?)

Amelie
Flags: needinfo?(dwilson)
Amelie,

All pending HWC patches are currently in bug 832383, including the patch that fixes the stretch issue. The plan is to uplift them to 1.0.1.

I cc'ed you in the bug. Let's continue the discussion there.
Flags: needinfo?(dwilson)
You need to log in before you can comment on or make changes to this bug.