Closed Bug 837236 Opened 11 years ago Closed 11 years ago

[B2G][Gallery] Video recording is not appearing in the Gallery app

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::Gallery
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:tef+, b2g18+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Status:
VERIFIED FIXED
Project Flags:
blocking-b2g tef+
Tracking Flags:
Tracking Status
b2g18 + fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: mlevin, Assigned: djf)

References

Details

(Keywords: regression, smoketest, unagi)

Attachments

(1 file, 2 obsolete files)

link to patch on github
108 bytes, text/html
daleharvey
: review+
Details 
Suite Name: Gallery
Test Case #: NA. Found during smoke testing.
Description: After recording a video the video does NOT appear in the Gallery.

Repro:
1) Install 20130201070203 unagi build
2) launch camera, and pick video recording.
3) Record a video.
3) stop the video recording, and launch gallery app

Expected:
video recording appears in gallery.

actual:
video recording does NOT appear in gallery, even after rebooting the Unagi.

NOTES:

The video DOES appear if User opens the video icon.

Unagi build: 20130201070203
Gecko: 51daa19d4045669303b77807d5832568e2d1f686
Gaia: 4d1087307c7377b1d4c7d72ba2e90e63cdb8fbc4
Kernel: Dec 5

Repro Rate:
3/3 on 3 different Unagi devices.
Depends on: 827311
Hrm.  it could be bug 836783?  if the app is already opened then it won't see the video... if the gallery app is closed before you switch over, it probably will appear?
wait.  2/1 build?  I'm not even seeing the video in gallery after taking a video... (which is different from bug 836783)...
I recall talking to Josh in Berlin WW... videos taken from the device are suppose to show in the gallery app.
blocking-b2g: --- → tef?
status-b2g18-v1.0.0: --- → affected
tracking-b2g18: --- → ?
Summary: [B2G][Gallery] Video recording is not appearing if you jump from camera to the gallery app → [B2G][Gallery] Video recording is not appearing in the Gallery app
mlevin and I talked it out.  ignore comment 1 please.  Videos are not being shown in the gallery app when taken from the camera, but they are being shown in the video app.
Still not working: 

gaia commit ID	00c0349
gecko commit ID	e2abfaf
This issue is reproing in: Smoke test build 
gaia:b277f372f097c77dd2ec1988a80e458280dda491
gecko:2aa2cbcc4e4bc667770eaeaad9c36efeadc1674a
Assignee: nobody → dflanagan
(In reply to Naoki Hirata :nhirata from comment #4)
> mlevin and I talked it out.  ignore comment 1 please.  Videos are not being
> shown in the gallery app when taken from the camera, but they are being
> shown in the video app.

Videos taken from the camera shown in Video app only, and any videos that been transferred to unagi also shown only in Video app, so we don't have any videos in Gallery now
Whiteboard: testrun 4
Whiteboard: testrun 4
What's happening is that when the Gallery app finds the new video, it tries to create a thumbnail for it by loading it into a <video> element.  That has always worked in the past, but now is triggering the onerror event handler, which is causing the "not a video file" message on the console.

Sometimes, the Camera app has the same problem itself: it has to create a thumbnail for the filmstrip, and sometimes we get the same "not a video file" message from the filmstrip code in Camera, caused by the very same on error handler.

The Video app, on the other hand, appears to (incorrectly?) ignore its onerror handler. It does get a thumbnail eventually, so the video apparently does load correctly after the error handler is called.

It seems to me that something has changed in the way gecko loads videos and that is causing the onerror callback to be called.  This looks like a regression in Gecko, maybe related to performance work?

Setting regressionwindow-wanted in the whiteboard in the hope that it will cause someone to bisect the bug and figure out which commit caused it.

I would guess that we could workaround this in gaia if necessary. But the right thing to do is probably to fix it in gecko.

This is a pretty bad regression; I think we need to block on it.
Keywords: regression, regressionwindow-wanted
Agree, we should block on this.  David, are you the best person to own this?
(In reply to Chris Lee [:clee] from comment #10)
> Agree, we should block on this.  David, are you the best person to own this?

I'm willing to own it, but might not be the best person. I could do some more investigation to see if I can figure out the nature of the gecko regression. I doubt I can fix that regression though. If I own it I'm more likely to work around the issue in Gaia.
I investigate the gecko regression.
The regression happened by Bug 834150. It forces always to use hw codec to decode vide. It disables to use sw video codecs at all. Software video codecs use a lot of pmen. But otoro and unagi devices do not have enough pmem for the software codecs.

Normal content process could not load hw codec, only privileged content process could load the hw codec. The application needs to have "deprecated-hwvideo" or "camera" to load hw codec. And Gallery app do not have "deprecated-hwvideo", then failed to load hw video codec.
By applying the patch, I confirmed that video thumbnails appear in Gallery app on unagi phone.
(In reply to Sotaro Ikeda [:sotaro] from comment #13)
> 
> Normal content process could not load hw codec, only privileged content
> process could load the hw codec. The application needs to have
> "deprecated-hwvideo" or "camera" to load hw codec. And Gallery app do not
> have "deprecated-hwvideo", then failed to load hw video codec.

This restriction is too strict. By disabling sw codec at all, only certified app could play H.264 video. The problem is discussed in Bug 793034. Bug 803471 provides a candidate implementation to remove the restriction.
Recent functional regression, we'll block. We should be looking for the lowest risk solution at this point (which sounds like the addition of the deprecated-hwvideo permission)
Assignee: dflanagan → sotaro.ikeda.g
blocking-b2g: tef? → tef+
tracking-b2g18: ? → +
Sotaro,

I'm trying to understand the scope of the bug. It looks like other apps (without the permission) can play .webm videos.  Is this bug specific to .3gp video from the camera?

If we just apply the permission fix to Gallery, as Alex suggests in comment 16, does that mean that other apps will not be able to play .3gp video?  What about web content running in the browser?

What would happen if we just removed the permission check for the hw video decoder and allowed all apps to use it without permission?  Does that risk system crashes?

Also: have you investigated the Camera app as well? In comment 9 I saw the same bug affecting the Camera.  I can't reproduce that this morning though.  Camera has "camera" permission obviously, but does not have deprecated-hwvideo.
The problem is only .3gp/.mp4 video speficic, because only these file formats use hw vodeo codecs. Normal app can play .webm videos(sw codec).

Current FirefoxOS load/open hw codecs driver within app processes. To open the driver, the app processes needs a special linux level permission. Normal content process do not have the permission to open the driver. This means normal app and web content running in the browser could not play .3gp/.mp4 video. They need to use WebActivity to play the video. But before applying Bug 834150, normal app and web content can play .3gp/.mp4 videos by using software codec...

"deprecated-hwvideo" and "camera" are very special app's permissions. They are used not only permission check, but also used to assign linux level groupid to app's processes. When an application has a "deprecated-hwvideo"/"camera" permission, gecko assigns following groupid(capabilities) to an app's process.

 - "deprecated-hwvideo" : AID_AUDIO, AID_CAMERA, AID_SDCARD_RW
 - "camera" : AID_AUDIO, AID_MEDIA

Followings are related source codes.

- http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#383

- http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/process_util_linux.cc#305
(In reply to David Flanagan [:djf] from comment #17)
>
> What would happen if we just removed the permission check for the hw video
> decoder and allowed all apps to use it without permission?  Does that risk
> system crashes?

If we want to permit any app and any web content to use hw codecs, we need to assign AID_MEDIA group id to all app processes in current architecture. It becomes a big security risk of FirefoxOS. It is not acceptable from security point of view. Bug 803471 addresses the problem by a safer way. But it is a very big change for FirefoxOS v1.0.

And we also need to think about hw resource consumption by anonymous web contents in the browser. A part of the problem is going to be handled in Bug 831747.
Ugh.

We can't let arbitrary apps access the hw decoders, because in v1.0 a compromised process gets direct access to DSPs and DSP memory.  (Which is why the "deprecated" permission exists.)
(In reply to David Flanagan [:djf] from comment #17)
>
> Also: have you investigated the Camera app as well? In comment 9 I saw the
> same bug affecting the Camera.  I can't reproduce that this morning though. 
> Camera has "camera" permission obviously, but does not have
> deprecated-hwvideo.

It seems that AID_CAMERA contains AID_MEDIA permissions in qcom's platform. Then Camera app can load hw codecs in Camera app's process.

I find a STR to reproduce comment 9.
 - 1. launching video app
 - 2. playing and pausing a video
 - 3. going to the camera.
 - 4. change to video mode.
 - 5. start recording and wait some seconds
 - 6. stop recording.
       thumbnail do not appear in filmstrip

Videp app still grabs hw video decoder, then Camera app failed to get hw video codec. There is no fallback to sw video codec now and failed to generate thumbnails.
This bug is causing the daily smoketests to fail. Sotaro, do you have an idea for how to fix this? Do you need additional help?
(In reply to Dietrich Ayala (:dietrich) from comment #22)
> This bug is causing the daily smoketests to fail. Sotaro, do you have an
> idea for how to fix this? Do you need additional help?

In which part did smoketests fail? If it is only Gallery app, we can apply attachment 710692 [details] [diff] [review]. But I never do commit to gaia. Can someone commit the change?
(In reply to Sotaro Ikeda [:sotaro] from comment #23)
> (In reply to Dietrich Ayala (:dietrich) from comment #22)
> > This bug is causing the daily smoketests to fail. Sotaro, do you have an
> > idea for how to fix this? Do you need additional help?
> 
> In which part did smoketests fail? If it is only Gallery app, we can apply
> attachment 710692 [details] [diff] [review]. But I never do commit to gaia.
> Can someone commit the change?

Anyway, it is necessary to commit attachment 710692 [details] [diff] [review] for v1.0. Because bug 803471 changes too much for v1.0 and the bug request more change to gecko for system resource handling.
(In reply to Sotaro Ikeda [:sotaro] from comment #21)
> (In reply to David Flanagan [:djf] from comment #17)
> 
> I find a STR to reproduce comment 9.
>  - 1. launching video app
>  - 2. playing and pausing a video
>  - 3. going to the camera.
>  - 4. change to video mode.
>  - 5. start recording and wait some seconds
>  - 6. stop recording.
>        thumbnail do not appear in filmstrip

attachment 713008 [details] do not solve the above STR. It is dupe of Bug 831747 and will be handled in the bug.
Comment on attachment 710692 [details] [diff] [review]
patch: add "deprecated-hwvideo" permission to Gallery app

Its a trivial patch. r+

After a detailed discussion on IRC with cjones, we've decided that landing it this way is the least bad option for v1.0.
Attachment #710692 - Flags: review+
(In reply to Sotaro Ikeda [:sotaro] from comment #25)
> Created attachment 713008 [details]
> Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8067

Is the pull request enough to get merged to gaia? Do I need to ask someone to merge the change?
Since we can't fix this at the platform level, I'm reassigning to myself to fix it in Gaia.

It turns out that just setting the deprecated-hwvideo permission is not enough to make this work because now we have three apps contending for the hardware decoder.

1) The gallery app has to be modified to release the hardware on mozvisibilitychange

2) The gallery app shouldn't try to create video thumbnails in the background because that is likely to run into hardware contention with the camera. Same for the video app, really. And this probably means that the camera app should be modified to create and save a thumbnail image along with the video.

3) the gallery app tries to display three <video> elements in its three frames. That won't work with only one hardware decoder. So it has to change to display three poster images, and only use an actual video element on the current one.
Assignee: sotaro.ikeda.g → dflanagan
(In reply to Sotaro Ikeda [:sotaro] from comment #28)
> (In reply to Sotaro Ikeda [:sotaro] from comment #25)
> > Created attachment 713008 [details]
> > Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8067
> 
> Is the pull request enough to get merged to gaia? Do I need to ask someone
> to merge the change?

Sotaro,

I was about to land it, but then discovered all the issues in comment 29.  So I'll work on a larger patch that addresses those and also sets deprecated-hwvideo
(In reply to David Flanagan [:djf] from comment #29)
> Since we can't fix this at the platform level, I'm reassigning to myself to
> fix it in Gaia.
> Sotaro,
> 
> I was about to land it, but then discovered all the issues in comment 29. 
> So I'll work on a larger patch that addresses those and also sets
> deprecated-hwvideo

Ok, thanks for the explanation.
Attachment #713008 - Attachment is obsolete: true
Sotaro, would it be possible to revert bug 834150, but *only* attempt to use the SW decoder for 3gp videos?
(In reply to Chris Jones [:cjones] [:warhammer] from comment #32)
> Sotaro, would it be possible to revert bug 834150, but *only* attempt to use
> the SW decoder for 3gp videos?

What is the intent of this? Does it mean that do not use hw codec at all and use sw codecs only?

It is possible to select sw codec by setting kSoftwareCodecsOnly flag to OMXCodec. But as in bug 834150, it consumes all pmems when decoding 720P. And andorid's standard sw codec support only H.264 baseline profile not main profile nor high profile.
(In reply to Sotaro Ikeda [:sotaro] from comment #33)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #32)
> > Sotaro, would it be possible to revert bug 834150, but *only* attempt to use
> > the SW decoder for 3gp videos?

One possible workaround to the problem is to use sw video codec as fallback only when video size is small enough. We can get video size information from MediaExtractor.
The goal would be to re-enable usage of SW decoder only for small 3gp videos.  But I hear that we're not 100% sure this won't regress stability metrics.
Chris,

Let me know how you'd like me to proceed here, Chris.  The options I've heard proposed are 

1) Re-enable SW decoding for 3gp videos as in comment 35.

2) Just decide that for v1.0, the Gallery app is photos only and can't catalog or play videos.  (I'm guessing that won't go over well with product management)

3) Workaround in Gaia.

Unless I hear otherwise, I'll plan to spend Wednesday and Thursday on #3.
If (3) is straightforward, we should do that.  Otherwise we'll need to do (1) which might regress stability metrics.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #37)
> If (3) is straightforward, we should do that.  Otherwise we'll need to do
> (1) which might regress stability metrics.

#3 is straightforward, but not small or simple because it involves changing Camera to save poster images and changing Gallery to display poster images instead of generating the video preview with <video> elements. I estimate 2 days of effort to work around in Gaia.
Product input could help here.
Flags: needinfo?(ffos-product)
UX and Product feel that there will be significant user confusion if user-recorded videos are not shown in the Gallery because of the direct button to access the gallery being next to the video record button.

We should continue to block on it and take the least risk option to return this functionality.
Flags: needinfo?(ffos-product)
Agree, let's continue down the option 3) path.
Have there been any updates here? This bug is blocking the youtube app for MWC.
Here's a big fix. I've kept it as 8 separate commits on github, thinking that might allow us to parallelize the review.  Will squash them before landing, of course.
Attachment #710692 - Attachment is obsolete: true
Attachment #714550 - Flags: review?(dale)
(In reply to thomas elin from comment #42)
> Have there been any updates here? This bug is blocking the youtube app for
> MWC.

There's been progress now.  I'm not sure how this bug will help with that one, though. This is mostly about the gallery app and videos recorded by the camera.  Are you sure this is the right bug to block on?  I guess you can try the patch and see?
I just thought of something that I haven't covered in my patch. If the Video app deletes a video that was recorded by the camera, it won't delete the new poster image, and Gallery will think that the video still exists. So Video has to change to delete both the video and its poster image before we land this.
I've updated the pull request with a new commit that makes Video delete the poster image along with video files.  Note that this required the addition of a new devicestorage permission for the app.  

Because of the new permission here (and the new deprecated-hwvideo permission for Gallery) be sure to do a reset-gaia before testing this patch.
Comment on attachment 714550 [details]
link to patch on github

I feel like there should probably be a bug open about having to do this in the first place at least for keeping track.

Good job, I have tested it as much as I can and there isnt any issues code wise that I can spot so far. I will continue to test as this is a big patch to be landing, but its the best way we have round the limitations we have and suggest landing it asap so we can get more testing done.
Attachment #714550 - Flags: review?(dale) → review+
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Landed on Gaia v1.0.1: https://github.com/mozilla-b2g/gaia/commit/4164424049f9caa4e6b26aefe3a4fa7421451b2b

There is probably a better way to do this, but I did it by creating a new pull request. Sorry if that messes things up.
Verified fix. recorded video appears now in gallery.

Gecko  http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/059a7e0badf7
Gaia   4164424049f9caa4e6b26aefe3a4fa7421451b2b
BuildID 20130217070201
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: