Closed Bug 897216 Opened 9 years ago Closed 9 years ago

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

Categories

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

x86
macOS
defect
Not set
normal

Tracking

(blocking-b2g:leo+, b2g18 verified, b2g-v1.1hd fixed)

RESOLVED FIXED
blocking-b2g leo+
Tracking Status
b2g18 --- verified
b2g-v1.1hd --- fixed

People

(Reporter: djf, Assigned: johnhu)

References

Details

(Keywords: regression)

Attachments

(9 files)

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.
John: do you have any idea about this bug? It looks like you've been the most active with this app recently.
Flags: needinfo?(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.
Flags: needinfo?(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
Assignee: nobody → johu
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???
Attachment #780320 - Flags: feedback?(igonzaleznicolas)
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 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>
Attachment #780320 - Attachment description: back out some code and fix the rotation and bug bug in view.js → back out some code and fix the rotation and size bug in view.js
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 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.
Attachment #780320 - Flags: feedback?(igonzaleznicolas) → review?(dflanagan)
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!
Attachment #780320 - Flags: review?(dflanagan) → review-
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.
Attachment #780320 - Flags: review- → review?(dflanagan)
blocking-b2g: --- → leo?
Keywords: regression
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.
Attachment #780320 - Flags: review?(dflanagan) → review+
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?
(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?
Status: NEW → ASSIGNED
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.
Sure, I can prepare a patch for only the patch of view.js for bug 904983.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
for uplift.
blocking-b2g: leo? → leo+
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
Flags: needinfo?(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.
Flags: needinfo?(johu) → needinfo?(jhford)
v1-train: 3b10fd7689a00990e4e45104a73e7948754628cc
Flags: needinfo?(jhford)
v1.1.0hd: 3b10fd7689a00990e4e45104a73e7948754628cc
v1.1.0hd: efe6256abe067580f6bb6f3ef1b42281c5e15d20
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
Duplicate of this bug: 904983
You need to log in before you can comment on or make changes to this bug.