Closed Bug 945508 Opened 6 years ago Closed 6 years ago

WebM video with 11:9 aspect ratio has bad playback layout

Categories

(Firefox OS Graveyard :: Gaia::Video, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3+, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

RESOLVED FIXED
1.3 C1/1.4 S1(20dec)
blocking-b2g 1.3+
Tracking Status
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: diego, Assigned: johnhu)

References

()

Details

(Whiteboard: [CR 580862])

Attachments

(4 files, 1 obsolete file)

Attached video Sample clip
No description provided.
Attached video Screen recording
Attached file Log
I created the sample clip by re-encoding meetthecubs.webm with ffmpeg:

$ ffmpeg -i meetthecubs.webm -aspect 11:9 -threads 0 output.webm

Is this a valid clip?
John,

Looks like you've been one the main video app contributors lately. Do you know what may be happening here?
Flags: needinfo?(johu)
Blocks: 930299
blocking-b2g: --- → 1.3?
Does this reproduce on 1.2 or 1.1?
Keywords: qawanted
Diego,

I found an error message from Log:
{file: "app://video.gaiamobile.org/index.html" line: 0 column: 54 source: "translate(2.842170943040401e-14px,203.63636363636365px) scale(1.0909090909090908)"}]

According to your video, the scaling of video is correct but the position is wrong. That may be caused by the above line. I will  tried to reproduce it in my site. I don't clear the need info flag to remind me to reproduce it.
blocking-b2g: 1.3? → 1.3+
The resolution of your device looks like 698.18181818x800. That looks so strange. According to the output.webm, the width of your device is 640 x 1.0909090909 = 698.181818 and the height of your deice is 360 x 1.090909090909 + 203.6363636363636363 x 2 = 800.

According to log, this is caused by the double precision issue. I cannot reproduce it with helix, inari, and zte open. But I can provide a possible patch for this, if you can help us to test on your device.
Flags: needinfo?(johu)
Attached patch bug945508.patch (obsolete) — Splinter Review
Hi Diego,

Please help to apply the patch here and retest in your device and give me some feedback about this patch. Thanks.
Attachment #8341492 - Flags: feedback?(dwilson)
Diego, this is a WIP patch and view.js is not included. If it works, I can make a final patch.
QA Contact: sarsenyev
I'm not sure what device is used for the test but the issue doesn't reproduce on Buri 1.2 COM RIL & Leo 1.1 COM RIL builds
The video layout is correct

Device:Buri 1.2 COM RIL build
BuildID: 20131203004002
Gaia: c8f14ad3950d59ba13d7639eff02d080060bb3ce
Gecko: 244e98241b2c
Version: 26.0
RIL version: 01.02.00.019.102
Firmware version: v1.2_20131115

Device: Leo 1.1 COM RIL
BuildID: 20131203041431
Gaia: 19c9ff3a46a4389e40253c97b359763243af4531
Gecko: 617eb9d9bcc2
Version: 18.0
RIL version: 01.01.00.019.281
Keywords: qawanted
QA Contact: sarsenyev
(In reply to sarsenyev from comment #10)
> I'm not sure what device is used for the test but the issue doesn't
> reproduce on Buri 1.2 COM RIL & Leo 1.1 COM RIL builds
> The video layout is correct
> 
> Device:Buri 1.2 COM RIL build
> BuildID: 20131203004002
> Gaia: c8f14ad3950d59ba13d7639eff02d080060bb3ce
> Gecko: 244e98241b2c
> Version: 26.0
> RIL version: 01.02.00.019.102
> Firmware version: v1.2_20131115
> 
> Device: Leo 1.1 COM RIL
> BuildID: 20131203041431
> Gaia: 19c9ff3a46a4389e40253c97b359763243af4531
> Gecko: 617eb9d9bcc2
> Version: 18.0
> RIL version: 01.01.00.019.281

Can you confirm reproduction on 1.3?
Keywords: qawanted
Looks like it's only seen on the developer JB phone I have with v1.3.

John's patch fixed it. Now I want to verify if the screen resolution is being reported correctly.
The bug doesn't reproduce on the latest 1.3 build on Buri.
The video layout is correct

Device: Buri 1.3 Central build
BuildID: 20131203040236
Gaia: 31808a29cfcffa584b6a88b4f1e515387f485a1b
Gecko: 8648aa476eef
Version: 28.0a1
Firmware version: v1.2_20131115
Keywords: qawanted
Attachment #8341492 - Flags: feedback?(dwilson) → feedback+
Whiteboard: [CR 580862]
This is caused by double precision problem in Computer world. What I did is just to trim the value smaller then 10^-9. That's really a extreme case. I believe we will meet the same problem in the future if we don't deal with this problem. So, I will create a patch for this.
Diego,
Please help us test this version. This includes all patches for all files.

David,
I had moved the setPlayerSize out as a utils function named fitContainer and use this utility function to handle both view.js and video.js.
Attachment #8341492 - Attachment is obsolete: true
Attachment #8342147 - Flags: review?(dflanagan)
Attachment #8342147 - Flags: feedback?(dwilson)
Assignee: nobody → johu
Diego,

Even if the screen resolution is reported correctly, we may still have chance to meet the same problem because this is a general double precision issue.

(In reply to Diego Wilson [:diego] from comment #12)
> Looks like it's only seen on the developer JB phone I have with v1.3.
> 
> John's patch fixed it. Now I want to verify if the screen resolution is
> being reported correctly.
Comment on attachment 8342147 [details] [review]
use parseFloat and toFixed to trim the value smaller than 10^-4

Retested on my device. LGTM
Attachment #8342147 - Flags: feedback?(dwilson) → feedback+
Status: NEW → ASSIGNED
Comment on attachment 8342147 [details] [review]
use parseFloat and toFixed to trim the value smaller than 10^-4

+1 for the VideoUtils file.

But r- because I don't understand why you need the parseFloat() calls.

And, actually, I don't understand what the bug was in the first place and why calling toFixed() fixes it.  Please add a comment explaining why it is necessary, or in the future someone might decide to remove that code.

Also, 9 digits after the decimal seems like overkill to me.  I think 4 would be plenty, wouldn't it?
Attachment #8342147 - Flags: review?(dflanagan) → review-
Ok, you are right. I forgot to tell the whole things out.

This bug is caused by the top/left of css transform cannot accept scientific notation, like 2.842170943040401e-14px. According to MDN[1], the translate value should be a number. That page doesn't say scientific notation is valid or not. So, I feel that it makes sense not to use the scientific notation.

The special screen resolution used by Diego Wilson creates this extreme case to make the inaccurate double value, like the result of 0.2 + 0.4. And this inaccurate value makes JavaScript to use scientific notation to show it.

When facing this kind of problem, we may use Math.round() to round the part we don't case. But I don't want to multiple 10^n and divide 10^n. That may make that value overflow. So, "toFixed()" is the choice to round the value at nth digit after decimal points. But the side effect is that it guarantees all digits are there even if they are all zero, like 1.500000000 just for 1.5. So, the translate value will be 1.500000000px. That looks ugly. That's why I use parseFloat to trim the 0 out.

I will add more comments to the patch.

And yes, 4 digits is enough. I will change that.

[1] https://developer.mozilla.org/en-US/docs/Web/CSS/number
Comment on attachment 8342147 [details] [review]
use parseFloat and toFixed to trim the value smaller than 10^-4

David,

I had changed the patch to have more explanation about parseFloat and toFixed fixed this bug. Please review it again.
Attachment #8342147 - Attachment description: use parseFloat and toFixed to trim the value smaller than 10^-9 → use parseFloat and toFixed to trim the value smaller than 10^-4
Attachment #8342147 - Flags: review- → review?(dflanagan)
Comment on attachment 8342147 [details] [review]
use parseFloat and toFixed to trim the value smaller than 10^-4

I disagree with you about the ugliness of the trailing zeros.  If you were writing a script that output a stylesheet for use elsewhere, I'd say that the parseFloat() step makes sense. But since you're just setting styles directly on an element and no human will ever read them, I'd just let the CSS parser deal with a few extra 0s.

But r+ to land it this way, if you want.
Attachment #8342147 - Flags: review?(dflanagan) → review+
John,

Looks like the change failed Travis automation in github. Are these results reliable?
Flags: needinfo?(johu)
> I disagree with you about the ugliness of the trailing zeros.  If you were
> writing a script that output a stylesheet for use elsewhere, I'd say that
> the parseFloat() step makes sense. But since you're just setting styles
> directly on an element and no human will ever read them, I'd just let the
> CSS parser deal with a few extra 0s.

Thanks for that. I will change the patch to remove the parseFloat parts.


(In reply to Diego Wilson [:diego] from comment #22)
> John,
> 
> Looks like the change failed Travis automation in github. Are these results
> reliable?

Diego,

I forgot to change the unit tests. Thanks for notifying me about that.
Flags: needinfo?(johu)
merged to master:
https://github.com/mozilla-b2g/gaia/commit/9a1e5fee6d68d7124ce84ce6b96dc63d73ad9085

All green on Travis.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Hi John,

Thanks landing this fix.

I it has not landed yet on the v1.3 branch of gaia. Can you propagate it please?
Flags: needinfo?(johu)
Flags: in-moztrap?
Target Milestone: --- → 1.3 C1/1.4 S1(20dec)
Flags: in-moztrap? → in-moztrap+
You need to log in before you can comment on or make changes to this bug.