798304 - Cant specify sub directories for videos in CameraControl API

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Dale Harvey (:daleharvey)]

Changing the file path from VID_0001.3gp to for instance DCIM/VID_0001.3gp when calling cameraObj.startRecording results in an error callback.

Comment 1

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

(In reply to [e-mail] from comment #0)
> Changing the file path from VID_0001.3gp to for instance DCIM/VID_0001.3gp
> when calling cameraObj.startRecording results in an error callback.

We made the decision to limit the filenames to -just- filenames to prevent apps from savings "images" like "../../system/b2g/b2g".  Sounds like that's not going to cut it anymore.

Comment 2

[David Flanagan [:djf]]

If you can make the camera save files though the devicestorage API that would be really nice because then interested apps (like the gallery) would be notified when a new video was saved.

Comment 3

[David Flanagan [:djf]]

If you have to have a hardcoded path, /sdcard/DCIM would be better than /sdcard/.  (But you'll have to ensure that DCIM gets created as needed.)

Comment 4

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

(In reply to David Flanagan [:djf] from comment #2)
> If you can make the camera save files though the devicestorage API that
> would be really nice because then interested apps (like the gallery) would
> be notified when a new video was saved.

Pictures are saved using the devicestorage API.  The challenge is with video, where the data being written is sourced by an AOSP component that needs a POSIX file handle--so startRecording() takes a deviceStorage object and a filename, builds a filesystem path, opens the file, and passes the handle to the recorder.

Comment 5

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

(In reply to David Flanagan [:djf] from comment #3)
> If you have to have a hardcoded path, /sdcard/DCIM would be better than
> /sdcard/.  (But you'll have to ensure that DCIM gets created as needed.)

The base path for recording video comes from calling deviceStorage('movies') (which I believe currently maps to /sdcard/Movies/).

The base path for pictures comes from deviceStorage('pictures') (which currently maps to /sdcard/DCIM).

Maybe the former should be changed to align with the latter?

Comment 6

[David Flanagan [:djf]]

Actually, unless doug has changed it back, device storage is now type (filename extension) based rather than directory based.  Both getDeviceStorage('videos') and getDeviceStorage('pictures') give access to all of /sdcard/ but they can only see files with certain extensions.  Since the videos are being recorded by the camera, we want to store them with the photos, like a digital camera would.  The gallery app is going to play them rather than the video app.  So having them in the DCIM/ directory will enable the Gallery and Video apps to distinguish them from other videos that are on the sdcard.

cc'ing dougt on this.

Comment 7

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

(In reply to David Flanagan [:djf] from comment #6)
> Actually, unless doug has changed it back, device storage is now type
> (filename extension) based rather than directory based.  Both
> getDeviceStorage('videos') and getDeviceStorage('pictures') give access to
> all of /sdcard/ but they can only see files with certain extensions.

Okay, I didn't understand that part of their behaviour (or it's new enough that I missed it).

I could just check the passed in filename for occurrences of "/../" and reject those, but what if someone creates a link to somewhere else in the filesystem, outside of /sdcard ?  (Assuming we support uSD cards formatted to ext* as well, and not just FAT*.)

Just found this:
http://pubs.opengroup.org/onlinepubs/009695399/functions/realpath.html

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Asking blocking-basecamp? since this prevent smoke tests from passing. CC'ing Tony and John.

Comment 9

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

Attachment: Fix to support camera app filenames that include subpaths

Comment 10

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

:dale, it's important to note that the gecko layer won't create any missing pathname components, so if you try to start video recording into '/sdcard/100MZFFO/VID_0001.3pg' and '/sdcard/100MZFFO' doesn't exist, start recording will fail.  The camera app is responsible for creating any intermediate folders.

Comment 11

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

(In reply to Mike Habicher [:mikeh] from comment #10)
> :dale, it's important to note that the gecko layer won't create any missing
> pathname components, so if you try to start video recording into
> '/sdcard/100MZFFO/VID_0001.3pg' and '/sdcard/100MZFFO' doesn't exist, start
> recording will fail.  The camera app is responsible for creating any
> intermediate folders.

(Assuming it can.  Thoughts, :dougt?)

Comment 12

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

try server push: https://tbpl.mozilla.org/?tree=Try&rev=9ea5a8b5af95

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dee0f227e656

Should this have a test?

Comment 14

[Ryan VanderMeulen [:RyanVM]]

I query on the checkin-needed flag, so make sure you set that. That said, the whiteboard comment is very useful (especially with b2g not having all the usual flags as I so nicely discovered in the other bug :P...)

Comment 15

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

(In reply to Ryan VanderMeulen from comment #14)
> I query on the checkin-needed flag, so make sure you set that. That said,
> the whiteboard comment is very useful (especially with b2g not having all
> the usual flags as I so nicely discovered in the other bug :P...)

Sorry--will do.  I'm still learning all this stuff.  :)

Comment 16

[Ryan VanderMeulen [:RyanVM]]

No worries, the whole process is a bit ad hoc anyway :)

https://hg.mozilla.org/releases/mozilla-aurora/rev/8df80d7202e8

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/dee0f227e656

Comment 18

[Dale Harvey (:daleharvey)]

> :dale, it's important to note that the gecko layer won't create any missing
> pathname components, so if you try to start video recording into '/sdcard/100MZFFO
> /VID_0001.3pg' and '/sdcard/100MZFFO' doesn't exist, start recording will 
> fail.  The camera app is responsible for creating any intermediate folders.

Sorry I should have paid more attention, reopening as it is not possible to create this directory from the camera app afaik

Either the startRecording API needs to ensure directory or I need something from the deviceStorage api to allow me to create them.

Comment 19

[Dale Harvey (:daleharvey)]

:dougt This issue also affects the ability to store photos taken, which uses the addNamed api from deviceStorage, as far as I can tell addNamed also doesnt ensure missing sub directories (I just attempted and it called the onerror callback)

If addNamed supported ensuring sub directories then I could store an empty blob in there before recording video, a little hacky but given the deviceStorage api it looks like the least hacky solution (I believe video recording over existing files is working, at least in quick tests)

Comment 20

[Dale Harvey (:daleharvey)]

dougt informed me that the deviceStorage API is supposed to create sub directories, which means I should be able to generate the nodes needed to save the video in, and if I cant I will refile a bug against the deviceStorage api, this one didnt need reopened.

Comment 21

[Paul Silaghi, QA [:pauly]]

Is this bug also related to Desktop Firefox ? If so, how can this fix be tested ?

Comment 22

[Jason Smith [:jsmith]]

(In reply to Paul Silaghi [QA] from comment #21)
> Is this bug also related to Desktop Firefox ? If so, how can this fix be
> tested ?

No it is not. This is the camera control API - b2g only.