Closed
Bug 996560
Opened 10 years ago
Closed 10 years ago
MaxFileSize should not restrict to 2GB for 1080p camera recording profile
Categories
(Firefox OS Graveyard :: Gaia::Camera, defect, P2)
Tracking
(blocking-b2g:1.4+, firefox29 wontfix, firefox30 fixed, firefox31 fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)
People
(Reporter: poojas, Assigned: vasanth)
Details
(Whiteboard: [CR 647174])
Attachments
(1 file, 1 obsolete file)
2.40 KB,
patch
|
poojas
:
review+
|
Details | Diff | Splinter Review |
We tried to enable 1080p recording as suggested in Bug 993969 Then for 1080p recording, android kk supports max file size upto 4GB whereas FFOS supports only upto 2GB It's because StageFrightRecorder::Prepare() enables 64BitFileOffset [1] whereas GonkRecorder::Prepare() doesn't do that [2] Shall we update GonkRecorder::Prepare() similar to #1? [1] https://www.codeaurora.org/cgit/quic/la/platform/frameworks/av/tree/media/libmediaplayerservice/StagefrightRecorder.cpp?h=b2g_kk_3.5#n790 [2] http://dxr.mozilla.org/mozilla-central/source/dom/camera/GonkRecorder.cpp line 658
Summary: Use 64 bit offsets for 1080p → Use 64 bit offsets for 1080p camera recording profile
Summary: Use 64 bit offsets for 1080p camera recording profile → MaxFileSize should not restrict to 2GB for 1080p camera recording profile
Comment 1•10 years ago
|
||
Happy to review a patch submission.
(In reply to Mike Habicher [:mikeh] from comment #1) > Happy to review a patch submission. One more query w.r.t to [1] is the check in StagefrightRecorder::prepare() considers mMaxFileDurationUs, which is always set to 0 in FFOS(?) Because of that even for low resolution recordings, maxFileSize will default to >2GB. Is that fine or do we need to change that condtion?
Comment 4•10 years ago
|
||
(In reply to Pooja from comment #2) > > One more query w.r.t to [1] is the check in StagefrightRecorder::prepare() > considers mMaxFileDurationUs, which is always set to 0 in FFOS(?) Because of > that even for low resolution recordings, maxFileSize will default to >2GB. > Is that fine or do we need to change that condtion? What are the implications of this? Unecessarily increased file size? If so, what's the correct value for mMaxFileDurationUs to ensure this doesn't happen?
Flags: needinfo?(mhabicher) → needinfo?(poojas)
Comment 5•10 years ago
|
||
Mike, Do you know if we support 1080p per design?
Flags: needinfo?(mhabicher)
Comment 6•10 years ago
|
||
(In reply to Preeti Raghunath(:Preeti) from comment #5) > > Do you know if we support 1080p per design? It depends on the device. Which one are we talking about?
Flags: needinfo?(mhabicher)
Comment 7•10 years ago
|
||
Yep, 1080p is supported.
Comment 9•10 years ago
|
||
(In reply to Mike Habicher [:mikeh] from comment #6) > (In reply to Preeti Raghunath(:Preeti) from comment #5) > > > > Do you know if we support 1080p per design? > > It depends on the device. Which one are we talking about? I know the Open C supports it. The device I was referring to was Madai. Sorry wasn't clear.
blocking-b2g: 1.4+ → 1.4?
Reporter | ||
Comment 10•10 years ago
|
||
(In reply to Mike Habicher [:mikeh] from comment #4) > (In reply to Pooja from comment #2) > > > > One more query w.r.t to [1] is the check in StagefrightRecorder::prepare() > > considers mMaxFileDurationUs, which is always set to 0 in FFOS(?) Because of > > that even for low resolution recordings, maxFileSize will default to >2GB. > > Is that fine or do we need to change that condtion? > > What are the implications of this? Unecessarily increased file size? If so, > what's the correct value for mMaxFileDurationUs to ensure this doesn't > happen? Implications could be unnecessary memory allocation. > what's the correct value for mMaxFileDurationUs to ensure this doesn't happen? For Resolution <720p Duration less then 30MIN MaxFileSize should(needed) be 2GB rest it should be 4GB Similarly resolution > 720p For duration less then 10MIN MaxFileSize should(needed) be 2GB rest it should be 4GB To overcome this we can modify condition check at [1] like: if (mVideoSource != VIDEO_SOURCE_LIST_END && mVideoEncoder != VIDEO_ENCODER_LIST_END && mVideoHeight && mVideoWidth && /*Video recording*/ mVideoHeight * mVideoWidth >= RES_720P ) i.e resolution >=720 MaxFileSize 4GB and resolution <720 MaxFileSize 2GB. Until FFOS not passing mMaxFileDurationUs thoughts? [1] https://www.codeaurora.org/cgit/quic/la/platform/frameworks/av/tree/media/libmediaplayerservice/StagefrightRecorder.cpp?h=b2g_kk_3.5#n790
Flags: needinfo?(poojas)
Updated•10 years ago
|
blocking-b2g: 1.4? → 1.4+
Comment 11•10 years ago
|
||
(In reply to Pooja from comment #10) > > thoughts? As long as we err on the side of being -more conservative- (see bug 987546 for an example of not being conservative) I think this is fine.
Reporter | ||
Comment 12•10 years ago
|
||
Reporter | ||
Comment 13•10 years ago
|
||
HI Mike, Wondering if you get a chance to review the patch?
Comment 14•10 years ago
|
||
Comment on attachment 8408879 [details] [diff] [review] Increase-maxFileSize-of-recorded-video.patch Review of attachment 8408879 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine to me. r+ with the log messages cleaned up; also, what is the "TODO" below? ::: dom/camera/GonkRecorder.cpp @@ +371,5 @@ > RE_LOGW("Target file size (%lld bytes) is too small to be respected", bytes); > } > > + if (bytes >= 0xffffffffLL) { > + RE_LOGW("Target file size (%lld bytes) too larger than supported, clip to 4GB", bytes); "Target file size (%lld bytes) too large to be respected, clipping to 4GB" @@ +664,5 @@ > status_t GonkRecorder::prepare() { > + if (mVideoSource != VIDEO_SOURCE_LIST_END && mVideoEncoder != VIDEO_ENCODER_LIST_END && > + mVideoHeight && mVideoWidth && // Video recording > + (mVideoHeight * mVideoWidth >= RES_720P)) { > + // TODO: Above check needs to be updated when mMaxFileDurationUs is set from camera app TODO ? @@ +665,5 @@ > + if (mVideoSource != VIDEO_SOURCE_LIST_END && mVideoEncoder != VIDEO_ENCODER_LIST_END && > + mVideoHeight && mVideoWidth && // Video recording > + (mVideoHeight * mVideoWidth >= RES_720P)) { > + // TODO: Above check needs to be updated when mMaxFileDurationUs is set from camera app > + RE_LOGV("File is huge so setting 64 bit file offsets"); "Video is high resolution so setting 64-bit file offsets"
Attachment #8408879 -
Flags: review?(mhabicher) → review+
Reporter | ||
Comment 15•10 years ago
|
||
Attachment #8410068 -
Flags: review+
Attachment #8408879 -
Attachment is obsolete: true
Reporter | ||
Comment 16•10 years ago
|
||
(In reply to Mike Habicher [:mikeh] from comment #14) > > + // TODO: Above check needs to be updated when mMaxFileDurationUs is set from camera app > > TODO ? As per comment 10, camera app doesn't set mMaxFileDurationUs. When it sets, we will need to update the condition similar to this [1] [1] https://www.codeaurora.org/cgit/quic/la/platform/frameworks/av/tree/media/libmediaplayerservice/StagefrightRecorder.cpp?h=b2g_kk_3.5#n794
Whiteboard: [CR 647174] → [CR 647174] [checkin-needed]
Comment 17•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/4ff01e095e0c
Whiteboard: [CR 647174] [checkin-needed] → [CR 647174]
Comment 18•10 years ago
|
||
Comment on attachment 8410068 [details] [diff] [review] Increase-maxFileSize-of-recorded-video.patch, v2 Make version explicit.
Attachment #8410068 -
Attachment description: Increase-maxFileSize-of-recorded-video.patch → Increase-maxFileSize-of-recorded-video.patch, v2
Comment 19•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4ff01e095e0c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S1 (9may)
Comment 20•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/4308d4e9ff77
status-b2g-v1.4:
--- → fixed
status-b2g-v2.0:
--- → fixed
status-firefox29:
--- → wontfix
status-firefox30:
--- → fixed
status-firefox31:
--- → fixed
Target Milestone: 2.0 S1 (9may) → 1.4 S6 (25apr)
Assignee | ||
Comment 21•10 years ago
|
||
Fails when >4GB remaining space (value more than 32 bits) is available in sdcard. CameraStartRecordingOptions.maxFileSizeBytes is long long [1] whereas StartRecordingOptions.maxFileSizeBytes is just uint32 [2] Hence in this assignment, [3] only lower 32 bits are assigned which is wrong. Ex: Free space: 4.1GB -> 0x108000000 -> 4429185024 Above assignment changes it to 0.1GB -> 0x08000000 -> 134217728 I will change StartRecordingOptions.maxFileSizeBytes type to uint64_t and upload here. [1] http://dxr.mozilla.org/mozilla-central/source/dom/webidl/CameraControl.webidl#95 [2] http://dxr.mozilla.org/mozilla-central/source/dom/camera/ICameraControl.h#113 [3] http://dxr.mozilla.org/mozilla-central/source/dom/camera/DOMCameraControl.cpp#765
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 22•10 years ago
|
||
This is an already landed patch - please file a followup for the issue you found.
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Comment 23•10 years ago
|
||
Not enough information with current STR to write test case to address bug.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(rmitchell)
Comment 24•10 years ago
|
||
This will need to be covered by a new test case.
Comment 25•10 years ago
|
||
This is most likely done in automation? No test case added to moztrap.
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(rmitchell)
Flags: in-moztrap-
You need to log in
before you can comment on or make changes to this bug.
Description
•