Cant specify sub directories for videos in CameraControl API

RESOLVED FIXED in Firefox 18

Status

()

defect
P1
critical
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: daleharvey, Assigned: mikeh)

Tracking

unspecified
mozilla19
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

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

Updated

7 years ago
Blocks: 794564
(In reply to dale@arandomurl.com 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.
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.
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.)
(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.
(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?
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.
(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
Reporter

Updated

7 years ago
Blocks: 796060
Asking blocking-basecamp? since this prevent smoke tests from passing. CC'ing Tony and John.
blocking-basecamp: --- → ?
Severity: normal → critical
Priority: -- → P1
Assignee: nobody → mhabicher
blocking-basecamp: ? → +
: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.
Status: NEW → ASSIGNED
(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?)
Attachment #669756 - Flags: review?(doug.turner) → review+
Whiteboard: needs-checkin-aurora
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...)
Keywords: checkin-needed
(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.  :)
No worries, the whole process is a bit ad hoc anyway :)

https://hg.mozilla.org/releases/mozilla-aurora/rev/8df80d7202e8
Keywords: checkin-needed
Whiteboard: needs-checkin-aurora
https://hg.mozilla.org/mozilla-central/rev/dee0f227e656
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Reporter

Comment 18

7 years ago
> :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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter

Comment 19

7 years ago
: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)
Reporter

Updated

7 years ago
Flags: needinfo?(doug.turner)
Reporter

Comment 20

7 years ago
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.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Flags: needinfo?(doug.turner)
Resolution: --- → FIXED
Is this bug also related to Desktop Firefox ? If so, how can this fix be tested ?

Updated

7 years ago
Whiteboard: [qa?]
(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.
Whiteboard: [qa?]
You need to log in before you can comment on or make changes to this bug.