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

Status: VERIFIED FIXED

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

Description

[sync-1]

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:

Comment 1

[buri.blff]

Attachment: Screen shot

Comment 2

[Michael Vines [:m1] [:evilmachines]]

Inder, can you please look at this?

Comment 3

[Diego Wilson [:diego]]

I've seen this before. It seems it's only a problem when Hardware composer is disabled

Comment 4

[Alex Keybl [:akeybl]]

Does this need to remain a blocker, given comment 3 and our planned config around hardware composer?

Comment 5

[Michael Vines [:m1] [:evilmachines]]

(ni on dwilson to further qualify comment 3)

Comment 6

[Inder]

diego, I could reproduce this issue on our device even if hw composer is enabled.

Comment 7

[Michael Vines [:m1] [:evilmachines]]

(Back to tef+ for now, until Inder is able to triage why these media file are causing trouble.)

Comment 8

[Inder]

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.

Comment 9

[Andrew Overholt [:overholt]]

CCing some media superstars for comment 8.

Comment 10

[Sotaro Ikeda [:sotaro]]

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.

Comment 11

[Sotaro Ikeda [:sotaro]]

(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.

Comment 12

[Sotaro Ikeda [:sotaro]]

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 13

[Sotaro Ikeda [:sotaro]]

Attachment: Part1 - add image size to SurfaceDescriptorGralloc

Comment 14

[Sotaro Ikeda [:sotaro]]

Attachment: Part2 - render gralloc buffer by using image size

Comment 15

[Sotaro Ikeda [:sotaro]]

Attachment: Part3 - set video size to SurfaceDescriptorGralloc

Comment 16

[Sotaro Ikeda [:sotaro]]

Comment on attachment 729792 [details] [diff] [review]
Part1 - add image size to SurfaceDescriptorGralloc

:nical, can you review the patch?

Comment 17

[Sotaro Ikeda [:sotaro]]

Comment on attachment 729793 [details] [diff] [review]
Part2 - render gralloc buffer by using image size

:roc, can you review the patch?

Comment 18

[Sotaro Ikeda [:sotaro]]

Comment on attachment 729794 [details] [diff] [review]
Part3 - set video size to SurfaceDescriptorGralloc

:doublec, can you review the patch?

Comment 19

[Sotaro Ikeda [:sotaro]]

I confirmed the patches fixed the problem on unagi phone.

Comment 20

[cajbir (:cajbir)]

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'.

Comment 21

[Nicolas Silva [:nical]]

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.

Comment 22

[Sotaro Ikeda [:sotaro]]

Attachment: Part1 v2 - add image size to SurfaceDescriptorGralloc

Commitable patch. carry "nical.bugzilla: review+".

Comment 23

[Sotaro Ikeda [:sotaro]]

Attachment: Part2 v2- render gralloc buffer by using image size

Commitable patch. Carry "roc: review+".

Comment 24

[Sotaro Ikeda [:sotaro]]

Attachment: Part3 v2 - set video size to SurfaceDescriptorGralloc

Committable patch. Carry "chris.double: review+".

Comment 25

[Sotaro Ikeda [:sotaro]]

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

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/345246c6d010
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae71a585726f
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ea012d36e73

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/345246c6d010
https://hg.mozilla.org/mozilla-central/rev/ae71a585726f
https://hg.mozilla.org/mozilla-central/rev/6ea012d36e73

Comment 28

[John Ford [:jhford] CET/CEST Berlin Time]

Is there a gaia component to this patch?

Comment 29

[Sotaro Ikeda [:sotaro]]

(In reply to John Ford [:jhford] from comment #28)
> Is there a gaia component to this patch?

IMHO, gaia side patch is not necessary.

Comment 30

[sarsenyev]

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 31

[Sotaro Ikeda [:sotaro]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #26)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/345246c6d010
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ae71a585726f
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6ea012d36e73

Can someone uplift the patches?

Comment 32

[Sotaro Ikeda [:sotaro]]

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

Comment 33

[Daniel Coloma:dcoloma]

(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.

Comment 34

[Sotaro Ikeda [:sotaro]]

Attachment: Part1 v2 for b2g18 - add image size to SurfaceDescriptorGralloc

Patch for b2g18. Carry "nical.bugzilla: review+"

Comment 35

[Sotaro Ikeda [:sotaro]]

Attachment: Part2 v2 for b2g18 - render gralloc buffer by using image size

Patch for b2g18. Carry "roc: review+"

Comment 36

[Sotaro Ikeda [:sotaro]]

Attachment: Part3 v2 for b2g18 - set video size to SurfaceDescriptorGralloc

Patch for b2g18. Carry "chris.double: review+"

Comment 37

[Sotaro Ikeda [:sotaro]]

Can I get help landing on gecko b2g18?

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/83276ffa8089
https://hg.mozilla.org/releases/mozilla-b2g18/rev/4e2faaa4b3dc
https://hg.mozilla.org/releases/mozilla-b2g18/rev/cb21c4153afa

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/a49d901c6f10
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/f4e509cb83cc
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/fff03f1901f2

Comment 39

[Carlos Martínez Toral [:carlosmartinez]]

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

Comment 40

[Amelie Kong]

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.

Comment 41

[Diego Wilson [:diego]]

Amelie,

There is a separater patch for hardware composer, which is enabled in that build. Please see bug 865112.

Comment 42

[Amelie Kong]

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

Comment 43

[Diego Wilson [:diego]]

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.