897216 - [video] Video app plays videos recorded by the camera sideways

Status: RESOLVED FIXED

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

Description

[David Flanagan [:djf]]

In gaia master (but not on v1-train) the video app plays videos recorded by the camera sideways, at least on my unagi device.

The thumbnails (generated by the camera) are correctly rotated, but the videos are not rotated.

I suspect that something has regressed the code that uses get_video_rotation.js, or that the CSS changes to support other resolutions regressed the CSS rotation used to orient the video correctly.

Comment 1

[David Flanagan [:djf]]

John: do you have any idea about this bug? It looks like you've been the most active with this app recently.

Comment 2

[John Hu [:johnhu][:johu][:醬糊小弟]]

I don't notice that. My device is current in v1-train because of OTA bug. I can test it with I finish the change of PR.

I had checked the change of css and get_video_rotation.js in github. There is no changes within these two weeks, except my css change. But that only changes about pictures and its related css.

Comment 3

[John Hu [:johnhu][:johu][:醬糊小弟]]

Got it. 

In this changeset [1], Ismael Gonzalez removed the code of video rotation at March 23th, 2013. I was just aboard within 1 months at that date. That's why I didn't notice that before.

I can put it back and keep the patch Ismael Gonzalez wrote.

[1] https://github.com/mozilla-b2g/gaia/commit/78f4ab86e4fb749df56e484ff59a709437623221

Comment 4

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: back out some code and fix the rotation and size bug in view.js

I think this bug is a regression of bug 874055. Bug 874055 describes the symptom is in the YouTube video, but he doesn't say which url. After investigation of the view.js of that version, view.js doesn't have the code to handle rotation, but video.js has. This is another code synchronization bug between view.js and video.js.

The patch of bug 874055 is trying to put the video in the middle of screen and make it fit width of screen. So, the video will always not rotate.

I don't know which url of bug 874055 uses. So, I can't reproduce it. I had made a quick patch to back out some of bug 874055 and fix the rotation and size bug in view.js.

Ismael Gonzalez, Is it possible to give the feedback of this patch before reviewing, since your are the reporter of that bug???

Comment 5

[John Hu [:johnhu][:johu][:醬糊小弟]]

Sorry. 

One thing forgot to mention:

The YouTube playback is changed to inline video element of browser or YouTube app. To test it, we may need to use bluetooth to send file to device. Event if we get the url of bug 874055, it may not test it.

The root is this one: https://bugzilla.mozilla.org/show_bug.cgi?id=843334.

This one pulls the trigger: https://bugzilla.mozilla.org/show_bug.cgi?id=887454. After the patch landed, the video is played inside browser.

Others:
https://bugzilla.mozilla.org/show_bug.cgi?id=886249#c19
https://bugzilla.mozilla.org/show_bug.cgi?id=891929

Comment 6

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 780320 [details]
back out some code and fix the rotation and size bug in view.js

>
><script>
>location.replace('https://github.com/mozilla-b2g/gaia/pull/11144');
></script>

Comment 7

[Ismael Gonzalez [:basiclines]]

Hey! I've just created that code in order to fix problems in hidpi devices when rotating videos from youtube (i didn't noticied that was creating a bug, my bad).
But as right now we have deviceCSSpx providing hidpi capabilities my patch is no longer needed, so it's ok backing out the patch.
Anyway, plx test on hidpi and non-hidpi devices this PR

Comment 8

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: hdpi landscape landscape-video in video.js

Comment 9

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: hdpi portrait landscape-video in video.js

Comment 10

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: hdpi landscape landscape-video view.js

Comment 11

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: hdpi portrait landscape-video in view.js

Comment 12

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: hdpi landscape portrait-video in video.js

Comment 13

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: hdpi portrait portrait-video in video.js

Comment 14

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: hdpi landscape portrait-video in view.js

Comment 15

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: hdpi portrait portrait-video in view.js

Comment 16

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 780320 [details]
back out some code and fix the rotation and size bug in view.js

I think this patch works both hdpi and non-hdpi devices. I had put 8 screenshot of video playing on hdpi devices with both video.js and view.js.

What I did is listed in the comment 4.

Comment 17

[David Flanagan [:djf]]

Comment on attachment 780320 [details]
back out some code and fix the rotation and size bug in view.js

John,

Thanks for going beyond the simple regression fix and also adding rotation support to the view activity. I didn't even realize that was missing.  Thanks also for testing all 8 possible cases.

The code looks solid to me. I'm giving r- only because I think it would be cleaner if you parsed the video rotation earlier in the initialization sequence, so that by the time setPlayerSize() is called, you already know what the rotation is.  I suggest a way you can do this on github.

(If you are really swamped with work, however, I can be convinced to land this patch the way it is.)

We should have a bug in the media team backlog to convert the video app and the view activity to use shared/js/video_player.js so that we don't have to maintain these two separate versions any longer!

Comment 18

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 780320 [details]
back out some code and fix the rotation and size bug in view.js

1. move the rotation parsing code at the initial place.
2. rename the id from fullscreen-view to videoFrame in view.html. I had tested this part. The player size will be calculated at setPlayerSize where player also enlarge the videoFrame. So, I don't think we need to have the same css style as fullscreen-view.

Comment 19

[David Flanagan [:djf]]

Comment on attachment 780320 [details]
back out some code and fix the rotation and size bug in view.js

Thanks for making these changes, John. They look great. I'm so behind on my reviews that I have not actually tested your patch. But if you have tested carefully, please land this.

Comment 20

[David Flanagan [:djf]]

Mike,

The bug that caused this regression never landed on v1-train, so the regression fix doesn't need to be leo.

However, John has added video rotation capability to the view activity, which was missing before. Is that why you're nominating it?

Comment 21

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to David Flanagan [:djf] from comment #20)
> 
> The bug that caused this regression never landed on v1-train, so the
> regression fix doesn't need to be leo.
> 
> However, John has added video rotation capability to the view activity,
> which was missing before. Is that why you're nominating it?

djf, sorry, I didn't realize that it never landed on v1-train.

But if this patch addresses the view activity rotation, it should be leo+, non?

Comment 22

[David Flanagan [:djf]]

I've landed John's patch to master: https://github.com/mozilla-b2g/gaia/commit/58c18428ed00d21d5b4105049c14c7ecc56b5f81

It looks like the changes to view.js in this patch will also fix bug 904983, so I'm landing this now so that that bug can be tested with this patch.

904983 seems like it might be a leo blocker, and if so, we'll need to uplift this patch. John, that might mean you'll have to prepare a patch that only includes the view.js changes and not the video.js changes.

Comment 23

[John Hu [:johnhu][:johu][:醬糊小弟]]

Sure, I can prepare a patch for only the patch of view.js for bug 904983.

Comment 24

[Preeti Raghunath(:Preeti)]

for uplift.

Comment 25

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

I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 58c18428ed00d21d5b4105049c14c7ecc56b5f81
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 26

[John Hu [:johnhu][:johu][:醬糊小弟]]

Hi John Ford, 

I had resolved the conflict in my repository:

https://github.com/huchengtw-moz/gaia/tree/video/Bug_897216_v1-train

Please help to continue uplift it. Thanks.

Comment 27

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

v1-train: 3b10fd7689a00990e4e45104a73e7948754628cc

Comment 28

[Tim Guan-tin Chien [:timdream] (please needinfo)]

v1.1.0hd: 3b10fd7689a00990e4e45104a73e7948754628cc
v1.1.0hd: efe6256abe067580f6bb6f3ef1b42281c5e15d20

Comment 29

[sarsenyev]

The issue no longer reproduces on Leo COM RIL 1.1
All videos play are correctly rotated as expected

Environmental  Variables:
Build ID: 20130904041204
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/307824edadd7
Gaia: d0a415bbf23e5d01c2b287d9fca708e167cfe70d
Platform Version: 18.1
RIL Version: 01.01.00.019.212
Firmware revision: D300f10a