812924 - Videos are recorded at 90deg

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Camera, defect, P1)

Comment 1

[Dale Harvey (:daleharvey)]

I opened a private tracking bug as I expect this will need some input from qualcomm

https://bugzilla.mozilla.org/show_bug.cgi?id=810424 is the original bug

Videos are currently recorded at a 90deg angle with a flag to tell players to rotate them back 90deg when playing, when played back on the device they are @ 90deg on the device. When transferred to the computer they play in almost all media players at a 90deg angle apart from quicktime which correctly rotates the video.

As mikeh mentioned in the previous bugs, aosp androids installed on this device have the same problem, although we have seen 1 video recorded that plays back everywhere with the correct rotation, however I dont know the source of the android.

It is a possible option to file this as a bug with the Firefox Media team  for the player to correctly respect the composition matrix, however the videos will still be 'broken' in all other media players apart from Quicktime

Comment 2

[Dale Harvey (:daleharvey)]

I opened a private tracking bug as I expect this will need some input from qualcomm

https://bugzilla.mozilla.org/show_bug.cgi?id=810424 is the original bug

Videos are currently recorded at a 90deg angle with a flag to tell players to rotate them back 90deg when playing, when played back on the device they are @ 90deg on the device. When transferred to the computer they play in almost all media players at a 90deg angle apart from quicktime which correctly rotates the video.

As mikeh mentioned in the previous bugs, aosp androids installed on this device have the same problem, although we have seen 1 video recorded that plays back everywhere with the correct rotation, however I dont know the source of the android.

It is a possible option to file this as a bug with the Firefox Media team  for the player to correctly respect the composition matrix, however the videos will still be 'broken' in all other media players apart from Quicktime

Comment 3

[Inder]

Setting video-param-rotation-angle-degrees causes the kKeyRotation(“rotA”) metadata to be set which is used by Android as well. Metadata based solution has the limitation that the video player will need to honor it. Don't know for sure but while digging about it I read that iPhone has the same problem. Only QuickTime can play the iPhone recorded videos with proper orientation.

I haven't yet found a solution where we could directly specify to the encoder to record video with correction orientation. Will update if I find something.

Comment 4

[Lawrence Mandel [:lmandel] (use needinfo)]

According to the dougt, this issue affects the unagi device but will not affect the production device. As such, basecamp-.

Comment 5

[Jason Smith [:jsmith]]

(In reply to Lawrence Mandel [:lmandel] from comment #4)
> According to the dougt, this issue affects the unagi device but will not
> affect the production device. As such, basecamp-.

This keeps getting flagged in our smoke tests and will affect dogfooders. I'm not sure if that warrants a minus. Smoke test blockers have to be fixed last time I checked. If it needs to be contested, then send the decision over to Tony. But as it stands right now, the argument given is insufficient until you talk to Tony.

Comment 6

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #5)
> (In reply to Lawrence Mandel [:lmandel] from comment #4)
> > According to the dougt, this issue affects the unagi device but will not
> > affect the production device. As such, basecamp-.
> 
> This keeps getting flagged in our smoke tests and will affect dogfooders.
> I'm not sure if that warrants a minus. Smoke test blockers have to be fixed
> last time I checked. If it needs to be contested, then send the decision
> over to Tony. But as it stands right now, the argument given is insufficient
> until you talk to Tony.

The main concern on my end is that there hasn't been agreement on the above note you've made with our team. And I'm not sure we can dump on dogfooders given the critical need for effective bug filing.

Comment 7

[Lawrence Mandel [:lmandel] (use needinfo)]

My understanding is that our hands our somewhat tied here. As it was explained to me, the issue is with a device driver that Mozilla does not develop and will not ship with v1. As such, this has a low priority to fix and is not critical to ship the device. I agree that any difference in functionality for our dogfooding program is regrettable, however, dogfooders are expecting a rough ride. We should tell them about this issue and why it won't be addressed on the device but I don't think we should go as far as to spend effort to fix an issue that, afaik, will not be present on the shipping device.

Comment 8

[Jason Smith [:jsmith]]

(In reply to Lawrence Mandel [:lmandel] from comment #7)
> My understanding is that our hands our somewhat tied here. As it was
> explained to me, the issue is with a device driver that Mozilla does not
> develop and will not ship with v1. As such, this has a low priority to fix
> and is not critical to ship the device. I agree that any difference in
> functionality for our dogfooding program is regrettable, however, dogfooders
> are expecting a rough ride. We should tell them about this issue and why it
> won't be addressed on the device but I don't think we should go as far as to
> spend effort to fix an issue that, afaik, will not be present on the
> shipping device.

What was proposed I think on a different thread (I need to remember what thread, maybe with UX?) was that although we might not "fix" unagi-specific issues like this one, we would inject a work-around to calm the dogfooding situation. That piece was argued it should block to maintain the sanity of the dogfooding experience, but not the full fix (equivalent to past bugs that said "let's stick a work-around in here, but not put the full fix in"). 

I still don't think the argument as it stands here argues doing nothing to maintain the dogfooding experience, but landing the full fix should indeed be a minus. 

Issues like these need a larger discussion between Lukas driving the dogfooding program, Tony driving QA, and various drivers. But at the moment, I still think we're jumping the gun here.

Comment 9

[Andreas Gal :gal]

This is not a blocker, since it doesn't affect the production device. We will of course take a reviewed patch to help out dogfooders.

Comment 10

[Jason Smith [:jsmith]]

Putting needinfo on Tony to figure out what to do here with these unagi-specific bugs against the current structure of our smoke tests.

Comment 11

[Jason Smith [:jsmith]]

Opps. Commented on the wrong bug and probably confused people. Anyways. Lets try this again.

If it affects otoro then Doug isn't right. qawanted to see if it happens on otoro, needsinfo on Doug, and renoming.

Comment 12

[Jason Smith [:jsmith]]

Ah. Well. Then this is a blocker then and Doug was wrong.

Comment 13

[Doug Turner (:dougt)]

let us stop talking about Doug being correct or not.  I think Doug would like to believe that Lawrence commented on the wrong bug.  In any case, re-nom'ing is the right idea - at least according to Doug.

Comment 14

[Jason Smith [:jsmith]]

(In reply to Doug Turner (:dougt) from comment #13)
> let us stop talking about Doug being correct or not.  I think Doug would
> like to believe that Lawrence commented on the wrong bug.  In any case,
> re-nom'ing is the right idea - at least according to Doug.

Sorry about the confusion. The bug you are probably referring to that's hardware specific with camera is probably bug 812208, which already got triaged to a minus.

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Doug Turner (:dougt) from comment #13)
> let us stop talking about Doug being correct or not.  I think Doug would
> like to believe that Lawrence commented on the wrong bug.  In any case,
> re-nom'ing is the right idea - at least according to Doug.

Entirely possible. Sorry this ended up reading like the blame Doug game. Thanks to Jason this is back on the nom list. We'll discuss it again on Monday.

Comment 17

[Dale Harvey (:daleharvey)]

Original bug was closed as this one had follow up info, it was blocking+ so this should be +'d too

Comment 18

[David Flanagan [:djf]]

Mike,

Can you point us again to the code that writes the rotation metadata?  Or point us to metadata documentation?

I've done a lot of metadata parsing, so I'm probably the best person to write the code that digs through the video files to find out if they need to be rotated, and I'll need to do this for the gallery app.

If this bug gets marked blocking and Mike tells me more about the metadata format, I'll take it and work on it.

Comment 19

[Tony Chung [:tchung]]

why did the blocking basecamp flag get renom'd?

Comment 20

[David Flanagan [:djf]]

Tony,

It sounds like there was miscommunication between Doug and Lawrence that caused it to be minused previously. Now we think that it needs to be fixed even on the hardware we will be shipping on, so it is nominated again.  (IIRC)

Comment 21

[Daniel Coloma:dcoloma]

810424 was a duplicated and was blocking-basecamp+, this should be a blocker too.

Comment 22

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

(In reply to David Flanagan [:djf] from comment #18)
> Mike,
> 
> Can you point us again to the code that writes the rotation metadata?  Or
> point us to metadata documentation?

Hi djf, sorry for the delay in getting back to you--my hard drive died (AGAIN*) so I'm playing catch-up while it's restoring.

The MPEG4 writer is:
http://androidxref.com/4.0.4/xref/frameworks/base/media/libstagefright/MPEG4Writer.cpp

The function that specifically writes the composition matrix (i.e. the rotation metadata) is writeCompositionMatrix():
http://androidxref.com/4.0.4/xref/frameworks/base/media/libstagefright/MPEG4Writer.cpp#560

-----
*I'm giving up on running Ubuntu in a vmware image, and am (finally!) going native.

Comment 23

[David Flanagan [:djf]]

Okay, we've got blocking+ and format information, so I'm taking this one.  I'll fix playback in the gallery.

Dale: is the plan still that the Video app won't play recorded videos?  If so, then Gallery is the only place we'll need to fix this.  On the other hand, the process of digging the transformation matrix out of the file will basically allow me to extract metadata as well, so if you want to add a metadata parser (to get video titles, e.g.) to the Video app, this will probably get you 90% of the way there.  (It looks like it is Quicktime format, so I can reuse metadata parsing code from the Musis app.)

Comment 24

[Chris Lee [:clee]]

Looks like we can play videos in both the Gallery and the Video app in today's build.  My understanding was that if this was fixed in the Video app, then it won't work in both Video and Gallery -- is this not the case?

Comment 25

[Dale Harvey (:daleharvey)]

We would need to copy the code across any place we play videos, even if we switch to only playing recorded videos in the gallery we should still be able to handle our own videos elsewhere (its just a matter of people copying videos files to play them in the video app)

As said I think this is the wrong hack, It will only fix videos played in apps which implement this parser (and quicktime) this means all videos uploaded to the internet and most played on the computer will be broken, it will also break when / if we actually fix the bug in our player and I dont know how our blob support works but it seems like parsing metadata of files which could be 100's of MB in web content would not work so well.

We know for certain that there is a proper fix for this, otherwise the video attached in https://bugzilla.mozilla.org/show_bug.cgi?id=810424 could not have been created, and if for some reason we could not apply a proper fix then at least fixing this in media is a much cleaner fix (the fact that a gaia hack breaks everything when we do fix the bug makes it a bad hack imo)

But actually doing those above 2 are past what I am able to do right now, I have tried pushing the relevant people into looking into this but havent heard anything back, so I guess this is the only choice...

Comment 26

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

(In reply to Dale Harvey (:daleharvey) from comment #25)
> 
> As said I think this is the wrong hack, It will only fix videos played in
> apps which implement this parser (and quicktime) this means all videos
> uploaded to the internet and most played on the computer will be broken....

From what I've read, YouTube also understands the composition matrix, and will rotate uploaded videos correctly.

Comment 27

[David Flanagan [:djf]]

(In reply to Dale Harvey (:daleharvey) from comment #25)
> We would need to copy the code across any place we play videos, even if we
> switch to only playing recorded videos in the gallery we should still be
> able to handle our own videos elsewhere (its just a matter of people copying
> videos files to play them in the video app)

I've got the code just about complete... Its just an extension to the metadata parser, so it shouldn't be too hard to integrate.

> 
> As said I think this is the wrong hack, It will only fix videos played in
> apps which implement this parser (and quicktime) this means all videos
> uploaded to the internet and most played on the computer will be broken, 

But at least they'll play correctly on our phone, now.  MikeH says that android has this same driver problem, so I don't think it is wise to hold our breath waiting for a driver fix.

it
> will also break when / if we actually fix the bug in our player 

Oh wait... You're saying we should fix it in gecko, aren't you?  That would be nice, but it still doesn't solve the problem of videos not playing right on the desktop in window media player, e.g. 

Anyway, I've got my fix mostly written, so I'm going to push ahead so at least we have a fix.

and I dont
> know how our blob support works but it seems like parsing metadata of files
> which could be 100's of MB in web content would not work so well.
> 

The metadata is near the front of the file and I'm slice() the blob into small bits before reading it.  Its reasonably efficient, I think.

> We know for certain that there is a proper fix for this, otherwise the video
> attached in https://bugzilla.mozilla.org/show_bug.cgi?id=810424 could not
> have been created, and if for some reason we could not apply a proper fix

So you're saying that would be a driver fix, right?

> then at least fixing this in media is a much cleaner fix (the fact that a
> gaia hack breaks everything when we do fix the bug makes it a bad hack imo)
> 

Fixing it "in media" means in gecko, right? Is anyone from the media team actually cc'ed on this bug?  Is there any chance that a change of that magnitude would be pushed into the beta tree?  If not, I don't think we have any choice in the matter.

> But actually doing those above 2 are past what I am able to do right now, I
> have tried pushing the relevant people into looking into this but havent
> heard anything back, so I guess this is the only choice...

I guess we agree on that :-)  I hope to have a fix later today.

Comment 28

[David Flanagan [:djf]]

(In reply to Chris Lee [:clee] from comment #24)
> Looks like we can play videos in both the Gallery and the Video app in
> today's build.  My understanding was that if this was fixed in the Video
> app, then it won't work in both Video and Gallery -- is this not the case?

The original design was that the Gallery app would play videos captured by the camera and the Video app would play other (downloaded? purchased?) videos. According to that (old) design, the fact that the Video app plays videos from the camera is a bug.

I am currently working on fixing this rotation bug in the Gallery.

As Dale points out, if a user does a bluetooth file transfer on a video and copies it to someone else's phone, then it won't be a "captured by the camera" video on that other phone, and it will appear in the Video app and not in the Gallery app.  This is pretty counterintuitive and it means that we have to apply the same (non-trivial) rotation fix to the Video app.

We've been told that our target audience has basically no data plan and little access to computers or wifi.  So the big question to me is... how are they going to get any videos on the phone for the Video app to play?

Here's a radical suggestion: what if ew just got ride of the video app entirely?
The gallery app displays all photos on the sdcard whether they come from the camera or not.  What if we made it display all videos on the sdcard as well, whether or not they come from the camera?

Comment 29

[David Flanagan [:djf]]

Attachment: link to github pull request

The attached pull request makes the gallery play videos at the correct oriention.
(It only affects newly recorded videos, though, unless you do a make reset-gaia.)

It does not affect the Video app at all. We need to decide what to do with that.

And it does not affect the way videos are played when you tap on the filmstrip.  If we're going to keep the filmstrip, we should change it to use the gallery rather than the video app.  But since we may be getting rid of it, I have not done that in this PR.

Comment 30

[Dale Harvey (:daleharvey)]

Comment on attachment 686333 [details]
link to github pull request

Merged in https://github.com/mozilla-b2g/gaia/commit/7efb38b2772d52c1b0b3883b6c4c334adb518fc9

Works well, cheers David.

As mentioned it doesnt resolve the issue (or the smoketest), the video app will need to copy these changes across if we still want the video app (and we will probably want to switch the filmstrip to open gallery, and filter DCIM out of the video app). But those decisions need to get made, I dont particulary want to start working on things that will just change on a whim later.

Comment 31

[David Flanagan [:djf]]

The Gallery app now plays recorded videos with the correct orientation.  We may also need to apply fixes in the Camera and Video apps, however.  

In order to close this SMOKETEST P1 CRITICAL BLOCKING+ bug, we need two decisions from UX and PM, so I'm needinfoing Josh and Chris here:

Decision #1: will videos recorded by the camera ever play in the Video app?  If so, we need to fix the video app. If not, we don't have to do that work. I propose that we make the video app never play videos from the camera or any video that even looks like it came from a camera (even when received via bluetooth transfer).  Those camera-like videos will always be played in Gallery, and the Video app will never have to worry about rotated videos. Josh and Chris: if you agree with this plan, we're done. If you disagree, then there are sub-decisions you'll have to make about just what each app plays.

Decision #2: will the Camera app include a filmstrip? If so, then we've got to change Camera to preview videos with the gallery (or to preview them natively).  If not, then there is no need for previews and the issue goes away.

Comment 32

[David Flanagan [:djf]]

Looks like I can only add one need info per comment. So now I'm flagging Chris Lee for info, too. Chris, please see comment #31

Comment 33

[David Flanagan [:djf]]

Okay, decisions made.

1) Both Chris and Casey say that the Video app should play all videos. The Gallery app will only play camera videos and will not attempt to play camera videos transferred by bluetooth.

2) Dale and I agreed, with Casey consulting, that the camera will include the preview filmstrip, but that we'll implement the previews natively for performance and so that it works from the lockscreen.

Clearing the needinfo requests.

Comment 34

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

So the remaining work here is to port the rotation to the video app?

Comment 35

[David Flanagan [:djf]]

Chris,

Porting to the video app remains, yes.

Also, the camera app is now going to display its own previews (because when using the camera from the lockscreen it can't use activities for previews) so that means that the rotation patch also needs to be ported to the camera.  Camera, Gallery and Video are going to end up sharing a bunch of code.  

I'm working on the camera piece of it currently.

Comment 36

[Faramarz Rashed]

Thanks :djf, resolving this would get us very close to passing all smoke-tests.  Don't let this out of your sight until it's resolved please.

Comment 37

[David Flanagan [:djf]]

Attachment: link to github pull request

Dale,

So this took longer than I expected, and is a bigger PR than I expected, but the result is pretty nice, I think.

See the overview in the github comments.  The trickiest part was getting the previews to display in the correct orientation even though this app locks its orientation.

The work for the video app should be much simpler than this. The key file is shared/js/media/get_video_rotation.js.

Comment 38

[David Flanagan [:djf]]

Comment on attachment 688159 [details]
link to github pull request

This patch give the camera the ability to delete photos and videos that it has just recorded. This means it needs new strings to ask the user whether they are sure.

Adding late-l10n flag and asking Stas for review.

Sorry!

Comment 39

[David Scravaglieri [:scravag]]

David, will we be able to see video well rotated in Video App ?

Comment 40

[Dale Harvey (:daleharvey)]

not from this commit, I am working on the similiar patch for the video app now, then wil review this one

Comment 41

[Dale Harvey (:daleharvey)]

Needs the bug number fixed before merging

Comment 42

[David Flanagan [:djf]]

Adding qawanted to this bug because the camera fix I just landed is pretty substantial.  Camera can now preview its own photos and videos natively (without activities) and can also share and delete them. So there's more UX to be aware of and test, and I want to get that on QA's radar.

Comment 43

[David Flanagan [:djf]]

Stas,

I forgot that I'd asked you to review the late l10n strings in https://github.com/mozilla-b2g/gaia/pull/6799 and I just landed it. Sorry!  If it wasn't a smoketest blocker, I'd back it out.

Will you still be able to review this and approve the new strings? The camera app can now delete photos and videos, so this just adds two confirmation messages, like "Delete photo?" and "Delete video?".  Gallery has messages just like this. I don't know if you have tools that will allow you to copy localizations from one app to another, but I'm guessing that you'll be able to use the unmodified localizations from Gallery.

Comment 44

[Jason Smith [:jsmith]]

John - Can you help or find someone to support testing David's patch?

Comment 45

[Dale Harvey (:daleharvey)]

Attachment: Path to github pull request (video app)

Comment 46

[David Flanagan [:djf]]

Comment on attachment 688572 [details]
Path to github pull request (video app)

r+ if you fix the one bug I found in your patch.  See my comments on github.

Comment 47

[Dale Harvey (:daleharvey)]

Merged in 

https://github.com/mozilla-b2g/gaia/commit/24b403d4951d3487d4c72bd73ef0e199bab914d9
https://github.com/mozilla-b2g/gaia/commit/b49bad50f370490a558070994df104fd46b8b967

Comment 48

[Staś Małolepszy :stas]

Comment on attachment 688159 [details]
link to github pull request

r=me.  Thanks :djf, and no worries about the early landing!