Closed Bug 1004434 Opened 6 years ago Closed 6 years ago

MaxFileSize, MaxVideoLength of CameraControl Interface recording options should be of 64 bits

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

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

RESOLVED FIXED
2.0 S2 (23may)
blocking-b2g 1.4+
Tracking Status
firefox30 --- wontfix
firefox31 --- wontfix
firefox32 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: vasanth, Assigned: vasanth)

References

Details

(Whiteboard: [caf priority: p2][CR 647174])

Attachments

(2 files, 5 obsolete files)

Follow up of bug 996560 comment 21 

That bug's fix 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 of maxFileSizeBytes are assigned which is wrong.

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
Attachment #8416325 - Flags: review?(mhabicher)
blocking-b2g: --- → 1.4?
Comment on attachment 8416325 [details] [diff] [review]
Bug1004434-MaxFileSize-MaxVideoLength-Camera-Recording-Options-64bit.patch

Review of attachment 8416325 [details] [diff] [review]:
-----------------------------------------------------------------

This is a good start, but it exposes a problem with nsGonkCameraControl::SetupRecording(), which currently takes signed integers:
http://dxr.mozilla.org/mozilla-central/source/dom/camera/GonkCameraControl.cpp#1540

Please fix this as well, and add a range check to SetupRecording() for file size and video length before setting the recorder properties; something like:
  if (x > INT64_MAX) {
    DOM_CAMERA_LOGE("max...=%llu is too big\n", x);
    return NS_ERROR_INVALID_ARG;
  }
Attachment #8416325 - Flags: review?(mhabicher) → review-
(In reply to Mike Habicher [:mikeh] from comment #2)
> This is a good start, but it exposes a problem with
> nsGonkCameraControl::SetupRecording(), which currently takes signed integers:
> http://dxr.mozilla.org/mozilla-central/source/dom/camera/GonkCameraControl.
> cpp#1540

I see later anyways GonkRecorder::SetParamMaxFileSizeBytes() clips this value to a max of 4GB [1]. 
Not sure an additional check is needed? 
We can just use int64_t instead of uint64_t for this params if you would like?

[1] http://dxr.mozilla.org/mozilla-central/source/dom/camera/GonkRecorder.cpp#374
Flags: needinfo?(mhabicher)
(In reply to vasanth from comment #3)
> (In reply to Mike Habicher [:mikeh] from comment #2)
> > This is a good start, but it exposes a problem with
> > nsGonkCameraControl::SetupRecording(), which currently takes signed integers:
> > http://dxr.mozilla.org/mozilla-central/source/dom/camera/GonkCameraControl.
> > cpp#1540
> 
> I see later anyways GonkRecorder::SetParamMaxFileSizeBytes() clips this
> value to a max of 4GB [1]. 
> Not sure an additional check is needed? 

Okay, that check is fine.

> We can just use int64_t instead of uint64_t for this params if you would like?

I don't think it makes sense to allow file sizes to be negative. I think the correct solution is to change maxFileSizeBytes and maxVideoLengthMs in CameraControl.webidl[2] to "unsigned long long" and making sure everything that calls down to GonkRecorder::SetParamMaxFileSizeBytes() consistently uses uint64_t. We can leave the final cast/ceiling operation to that function.

> [1]
> http://dxr.mozilla.org/mozilla-central/source/dom/camera/GonkRecorder.cpp#374

2. http://dxr.mozilla.org/mozilla-central/source/dom/webidl/CameraControl.webidl#95
Flags: needinfo?(mhabicher)
(In reply to Mike Habicher [:mikeh] from comment #4)
> I don't think it makes sense to allow file sizes to be negative. I think the
> correct solution is to change maxFileSizeBytes and maxVideoLengthMs in
> CameraControl.webidl[2] to "unsigned long long" and making sure everything that calls down to 
> GonkRecorder::SetParamMaxFileSizeBytes() consistently uses uint64_t.

I agree that file size can't be -ve. GonkRecorder::SetParamMaxFileSizeBytes() is called by GonkRecorder::SetParameter() which uses only signed (32/64) integers for all params. [1] 
So just modifying max file size to uint64_t won't be consistent in this way? 
In this case, only size matters here and not the type. 
So shall we go ahead with the existing patch?

[1] http://dxr.mozilla.org/mozilla-central/source/dom/camera/GonkRecorder.cpp#513
Flags: needinfo?(mhabicher)
blocking-b2g: 1.4? → 1.4+
(In reply to vasanth from comment #5)
> 
> I agree that file size can't be -ve.
> GonkRecorder::SetParamMaxFileSizeBytes() is called by
> GonkRecorder::SetParameter() which uses only signed (32/64) integers for all
> params. [1] 
> So just modifying max file size to uint64_t won't be consistent in this way? 
> In this case, only size matters here and not the type. 
> So shall we go ahead with the existing patch?

I've just looked at the code, and it's worse than that. GonkRecorder::setParameter() is broken. Although it uses strtoll() to parse the string argument, which checks that the argument will fit into a |long long|, in the case of video time limit, the code then blindly multiplies that value by 1000LL, which can overflow the int64_t into something much smaller.

e.g. maxVideoLengthMs = 0x2000000000000067 * 1000 = 0x19258, a synthetic case, but entirely wrong behaviour.

Either we need to fix GonkRecorder::SetParameter(), or make sure nsGonkCameraControl::SetupRecording() is smart enough to prevent this.
Flags: needinfo?(mhabicher)
A patch that uses unsigned 64-bit integers, and sanity checks them for the GonkRecorder.
Attachment #8418816 - Flags: feedback?(vasanth)
This update adds [Clamp] to the dictionary members, so that values outside the range of the 'unsigned long long' are clamped to 0..ULLONG_MAX.
Attachment #8416325 - Attachment is obsolete: true
Attachment #8418816 - Attachment is obsolete: true
Attachment #8418816 - Flags: feedback?(vasanth)
Attachment #8418956 - Flags: feedback?(vasanth)
Comment on attachment 8418956 [details] [diff] [review]
Make video limits use uint64_t, with sanity checks [b2g-inbound] v2

Verified it works. Doesn't apply on V1.4, please upload the rebased patch for V1.4 as well
Attachment #8418956 - Flags: feedback?(vasanth) → feedback+
Attachment #8418956 - Attachment description: Make video limits use uint64_t, with sanity checks, v2 → Make video limits use uint64_t, with sanity checks [b2g-inbound] v2
Attachment #8418956 - Flags: review?(dhylands)
Attachment #8419448 - Flags: feedback?(vasanth) → feedback+
Comment on attachment 8418956 [details] [diff] [review]
Make video limits use uint64_t, with sanity checks [b2g-inbound] v2

bz, would you mind looking over the WebIDL changes (again)?
Attachment #8418956 - Flags: review?(bzbarsky)
Comment on attachment 8418956 [details] [diff] [review]
Make video limits use uint64_t, with sanity checks [b2g-inbound] v2

r=me
Attachment #8418956 - Flags: review?(bzbarsky) → review+
Hema

Can someone else review for Dave. Looks like he is away too.
Flags: needinfo?(hkoka)
Sotaro,

I think Mike probably already reached out to you on Friday for review before he left on vacation. Could you please help here?

Thanks
Hema
Flags: needinfo?(hkoka) → needinfo?(sotaro.ikeda.g)
Attachment #8418956 - Flags: review?(dhylands) → review?(sotaro.ikeda.g)
Comment on attachment 8418956 [details] [diff] [review]
Make video limits use uint64_t, with sanity checks [b2g-inbound] v2

Review of attachment 8418956 [details] [diff] [review]:
-----------------------------------------------------------------

Just a small nit, otherwise this looks good to me.

::: dom/camera/GonkCameraControl.cpp
@@ +1557,3 @@
>  
> +  DOM_CAMERA_LOGI("maxVideoLengthMs=%llu\n", aMaxVideoLengthMs);
> +  const int64_t kMaxVideoLengthLimit = INT64_MAX / 1000;

nit: Rather than Length, I think that using Millisecs (or microsecs) would be more useful (Length doesn't give any sense of units).

At the very least a comment would be nice.

nit: kMaxVideoLengthLimit should be a uint64_t so that we don't get warnings about mixing signed/unsigned types.
Attachment #8418956 - Flags: review?(sotaro.ikeda.g) → review+
Thanks Dave!
Flags: needinfo?(sotaro.ikeda.g)
Hema -- considering MikeH is on PTO who is addressing the nit Dave highlighted? Do you want Vasanth to take care of it?
Flags: needinfo?(hkoka)
I think it would be fine to address the nits in a followup bug. I guess the other question to ask is who will be landing this?
Taking Hema's ni:

Dave

Can you please land this? Not sure if partner should.
Flags: needinfo?(hkoka)
Attachment #8418956 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/9619b6278c49
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S2 (23may)
Whiteboard: [CR 647174] → [caf priority: p2][CR 647174]
No STR is present to create test case to address bug.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(rmitchell)
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.