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)

ARM
Gonk (Firefox OS)

Tracking

(blocking-b2g:1.4+, firefox29 wontfix, firefox30 fixed, firefox31 fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
1.4 S6 (25apr)
blocking-b2g 1.4+
Tracking Status
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)

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
blocking-b2g: --- → 1.4?
Whiteboard: [CR 647174]
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
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?
Ni mike for pooja's question
Flags: needinfo?(mhabicher)
(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)
Mike,

Do you know if we support 1080p per design?
Flags: needinfo?(mhabicher)
(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)
Yep, 1080p is supported.
CS blocker - so blocking+
blocking-b2g: 1.4? → 1.4+
(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?
(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)
blocking-b2g: 1.4? → 1.4+
(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.
Assignee: nobody → poojas
Status: NEW → ASSIGNED
Attachment #8408879 - Flags: review?(mhabicher)
HI Mike,
Wondering if you get a chance to review the patch?
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+
Attachment #8410068 - Flags: review+
Attachment #8408879 - Attachment is obsolete: true
(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]
https://hg.mozilla.org/integration/b2g-inbound/rev/4ff01e095e0c
Whiteboard: [CR 647174] [checkin-needed] → [CR 647174]
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
https://hg.mozilla.org/mozilla-central/rev/4ff01e095e0c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S1 (9may)
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 → ---
Assignee: poojas → vasanth
This is an already landed patch - please file a followup for the issue you found.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Not enough information with current STR to write test case to address bug.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(rmitchell)
This will need to be covered by a new test case.
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.