776062 - Camera - video recording support

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

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

Need to integrate video recording patches.

Comment 1

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

Attachment: WIP - new CameraControl API with video recording support

This monolithic patch does not require anything from bug 740997 (at least until that lands) to be used.  Just patch and build and go.

Comment 2

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

Attachment: WIP - new CameraControl API with video recording support (INCREMENTAL)

As requested, an incremental version of the patch that can (and must) be applied on top of the patch in bug 740997.

Comment 3

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

Attachment: WIP - new CameraControl API with video recording support

Rebase of the patch, based on the latest from bug 740997.  This monolithic patch includes EVERYTHING from that bug as well, so it can be applied to a base tree.

Comment 4

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

Attachment: WIP - new CameraControl API with video recording support

Missed some wrong symbols in dictionary_helper_gen.conf, though it didn't seem to affect the build any.

Comment 5

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

Attachment: WIP - new CameraControl API with video recording support (INCREMENTAL)

Incremental version of the patch; apply on top of the WIP patch from bug 740997.

Comment 6

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

Attachment: WIP - video recording support

Comment 7

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

Attachment: camera.js that exercises the recording API

Comment 8

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

doublec: review ping?

Comment 9

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

Attachment: WIP - video recording support

Rebased.

Comment 10

[cajbir (:cajbir)]

Comment on attachment 654324 [details] [diff] [review]
WIP - video recording support

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

You'll need to split your patch up so you can get review from the right parties. configure.in will need a build peer for example. I'm not a DOM peer myself so I'm not sure I'm the best person to be reviewing this as I don't see much in the way of video/audio media code but I'll do a preliminary review anyway. 

Do some files come from Android source? I see their copyright header. If you could put these in a separate patch and provide some way of updating them to latest Android source, or showing the differences between them and the original Android version. The media/omx-plugin code provides a shell script and patch file for example. 

I did not look at any of the "#if 0" blocks of code. I assume these will be removed rather than enabled.

::: dom/camera/GonkCameraControl.cpp
@@ +614,5 @@
>  
> +nsresult
> +nsGonkCameraControl::SetupVideoMode()
> +{
> +  // read perferences for video mode

Spelling of 'perferences'

@@ +634,5 @@
> +
> +  PullParametersImpl(nullptr);
> +
> +  // set params with new values
> +  const size_t SIZE = 256;

Why was the magic number '256' chosen? Add a comment.

@@ +694,5 @@
> +  }
> +  return rv;
> +}
> +
> +static int

Add a comment to describe what the return value is.

@@ +695,5 @@
> +  return rv;
> +}
> +
> +static int
> +GetFileNameWithDate(char* outstr, int maxlen)

I'm not familiar with the style guide requirements for DOM code but everywhere else I've done stuff for Gecko parameter names need to be like aOutstr, aMaxlen, etc. This goes for all the parameter names used in this and other files.

@@ +703,5 @@
> +  int actualLen;
> +
> +  t = time(NULL);
> +  tmp = localtime(&t);
> +  if (tmp == NULL) {

Restructure to declare variable and assign at the same time as is the idiom for C++. eg:

time_t = time(NULL);
struct tm* tmp = localtime(&t);
etc

@@ +725,5 @@
> +  }
> +  return ".3gp";
> +}
> +
> +static int

Comment documenting what the return value is.

@@ +734,5 @@
> +  char fileName[MAX_VIDEO_FILE_NAME_LEN];
> +  char fileName2[MAX_VIDEO_FILE_NAME_LEN];
> +  char* videoFileAbsPath;
> +  struct stat buffer;
> +  int result;

Declare the variables at the point you first use them rather than the C approach of declaring them first at the top of the function.

@@ +742,5 @@
> +    strncpy(fileName, DEFAULT_VIDEO_FILE_NAME, MAX_VIDEO_FILE_NAME_LEN);
> +  }
> +
> +  // add the extension
> +  strncat(fileName, GetFileExtension(nVideoFileFormat), MAX_VIDEO_FILE_NAME_LEN);

If the result of strlen(GetFileExtension(nVideoFileFormat)) + strlen(fileName) > MAX_VIDEO_FILE_NAME_LEN then this will write out of bounds of fileName. The third parameter bounds the size of the second, not the first.

Ideally you should use Mozilla's string handling APIs to avoid these issues rather than C functions.

@@ +747,5 @@
> +
> +  DOM_CAMERA_LOGI("Video File Name: \"%s\" \n", fileName);
> +
> +  // check if the video storage dir exists
> +  result = stat(VIDEO_STORAGE_DIR, &buffer);

Why use 'stat' and not nsIFile routines?

@@ +774,5 @@
> +#endif
> +  }
> +
> +  DOM_CAMERA_LOGI("Opening video file: %s\n", videoFileAbsPath);
> +  int fd = open(videoFileAbsPath, O_RDWR | O_CREAT, 0744);

Why use C routines like 'open' instead of equivalent Mozilla API's (nsIFile, etc)?

@@ +796,5 @@
> +
> +nsresult
> +nsGonkCameraControl::SetupRecording()
> +{
> +  const size_t SIZE = 256;

Comment explaining why this magic value was picked.

@@ +831,5 @@
> +  int fd = CreateVideoFile(mVideoFile, mVideoFileFormat);
> +  if (fd < 0) {
> +    return NS_ERROR_FAILURE;
> +  }
> +  CHECK_SETARG(mRecorder->setOutputFile(fd, 0, 0));

Why are file descriptors used instead of Mozilla file api functions? Is it a requirement of interfacing to the Android code?

::: dom/camera/GonkCameraControl.h
@@ +77,5 @@
> +  PRUint32                  mVideoWidth;
> +  PRUint32                  mVideoHeight;
> +  nsString                  mVideoFile;
> +
> +  // video mode settings

Add comments for each of these additional members. What unit is 'mDuration'. What do the values of 'mVideoFileFormat' mean, etc.

::: dom/camera/GonkCameraHwMgr.cpp
@@ +164,5 @@
>  void
> +GonkCameraHardware::DataCallbackTimestamp(nsecs_t timestamp, int32_t aMsgType, const sp<IMemory> &aDataPtr, void* aUser)
> +{
> +  DOM_CAMERA_LOGI("%s",__func__);
> +  GonkCameraHardware* hw = GetHardware((PRUint32)aUser);

Use reinterpret_cast here and other places you do this cast.

@@ +499,5 @@
> +int
> +GonkCameraHardware::StartRecording(PRUint32 aHwHandle)
> +{
> +  DOM_CAMERA_LOGI("%s: aHwHandle = %d\n", __func__, aHwHandle);
> +  int rv = OK;

s/int/status_t/ I think.

::: dom/camera/GonkCameraHwMgr.h
@@ +48,4 @@
>    ~GonkCameraHardware();
>    void init();
>  
> +  static void                   DataCallbackTimestamp(nsecs_t timestamp, int32_t aMsgType, const sp<IMemory>& aDataPtr, void* aUser);

s/timestamp/aTimestamp/

::: dom/camera/GonkCameraListener.h
@@ +11,5 @@
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */

I notice this is new code but uses the Android license not the MPL. I don't know what the licensing requirements are for gecko code for this sort of thing but thought I should point it out.

@@ +25,5 @@
> +// ref-counted object for callbacks
> +class GonkCameraListener: virtual public RefBase
> +{
> +public:
> +    virtual void notify(int32_t msgType, int32_t ext1, int32_t ext2) = 0;

aMsgType, aExt1, etc.

::: dom/camera/GonkCameraPreview.cpp
@@ +26,4 @@
>  
>  using namespace mozilla;
>  
> +#define YUV_CONVERT_INPLACE   0

Comments explaining what this does and what values it can have.

::: dom/camera/GonkCameraSource.cpp
@@ +76,5 @@
> +    : mSource(source) {
> +}
> +
> +GonkCameraSourceListener::~GonkCameraSourceListener() {
> +}

Other code in this patch puts the opening brace for a function on the following line. Please do that here, or change all the other usage.

@@ +103,5 @@
> +    }
> +}
> +
> +static int32_t getColorFormat(const char* colorFormat) {
> +    return OMX_COLOR_FormatYUV420SemiPlanar;

You have dead code following this return.

@@ +279,5 @@
> +
> +/*
> + * If the preview and video output is separate, we only set the
> + * the video size, and applications should set the preview size
> + * to some proper value, and the recording framework will not

"...set the the video size..." <-- duplicated "the"

@@ +299,5 @@
> +static void getSupportedVideoSizes(
> +    const CameraParameters& params,
> +    bool *isSetVideoSizeSupported,
> +    Vector<Size>& sizes) {
> +

Why is the isSetVideoSizeSupported a pointer to a boolean? Why not make it the return value of the function?

@@ +376,5 @@
> +        const char* supportedFrameRates =
> +                params->get(CameraParameters::KEY_SUPPORTED_PREVIEW_FRAME_RATES);
> +        CHECK(supportedFrameRates != NULL);
> +        LOGV("Supported frame rates: %s", supportedFrameRates);
> +        char buf[4];

Why the magic value '4' here?

@@ +401,5 @@
> +            LOGE("Could not change settings."
> +                 " Someone else is using camera %p?", mCamera.get());
> +#else
> +        if (OK != GonkCameraHardware::PushParameters(mCameraHandle,*params)) {
> +            LOGE("Could not change settings."

In some places in this patch you use reverse conditional checks (some_constant == some_var) instead of (some_var == some_constant). In other places you didn't. Please standardise on one usage. I'd prefer the latter.

@@ +622,5 @@
> +      LOGW("Invalid hfr value(%d) set from app. Disabling HFR.", hfr);
> +      hfr = 0;
> +    }
> +
> +    int64_t glitchDurationUs = (1000000LL / mVideoFrameRate);

Can mVideoFrameRate be zero here? If so you'll want to handle the divide by zero case.

@@ +742,5 @@
> +            mCamera->disconnect();
> +        }
> +        mCamera->unlock();
> +        mCamera.clear();
> +        mCamera = 0;

Should this '0' be nsnullptr?

@@ +771,5 @@
> +    releaseQueuedFrames();
> +    while (!mFramesBeingEncoded.empty()) {
> +        if (NO_ERROR !=
> +            mFrameCompleteCondition.waitRelative(mLock,
> +                    mTimeBetweenFrameCaptureUs * 1000LL + CAMERA_SOURCE_TIMEOUT_NS)) {

Can this calculation overflow? If so you should check and handle it. It probably depends if mTimeBetweenFrameCaptureUs is bounded previously.

::: dom/camera/GonkCameraSource.h
@@ +148,5 @@
> +        virtual void binderDied(const wp<IBinder>& who);
> +    };
> +#endif
> +
> +    enum CameraFlags {

Document what these flags mean.

@@ +158,5 @@
> +    Size     mVideoSize;
> +    int32_t  mVideoFrameRate;
> +    int32_t  mColorFormat;
> +    status_t mInitCheck;
> +

Comments explaining what these members are and what units they are in. Same with all the other members in the class that aren't documented.

@@ +174,5 @@
> +    bool mStarted;
> +    int32_t mNumFramesEncoded;
> +
> +    // Time between capture of two frames.
> +    int64_t mTimeBetweenFrameCaptureUs;

What unit is this? Seconds, milliseconds, etc?

::: dom/camera/GonkRecorder.cpp
@@ +98,5 @@
> +}
> +#endif
> +
> +#if 1
> +static sp<IOMX> sOMX = NULL;

This is the same as what's in the media plugin code and I believe Edwin's "optimized stagefright" patch has it also. How many static OMX's are we going to have? Should this be factored out so that one is shared? Actually that's what I thought OMXClient was for. Can we use that?

@@ +217,5 @@
> +    }
> +
> +    // Use default values if appropriate setparam's weren't called.
> +    if(mAudioEncoder == AUDIO_ENCODER_AAC) {
> +        mSampleRate = mSampleRate ? mSampleRate : 48000;

Explain why these default values were chosen.

@@ +221,5 @@
> +        mSampleRate = mSampleRate ? mSampleRate : 48000;
> +        mAudioChannels = mAudioChannels ? mAudioChannels : 2;
> +        mAudioBitRate = mAudioBitRate ? mAudioBitRate : 156000;
> +    }
> +    else{

Explain why these default values were chosen.

@@ +322,5 @@
> +
> +    if (mOutputFd >= 0) {
> +        ::close(mOutputFd);
> +    }
> +    mOutputFd = dup(fd);

Why are we using file descriptors and not our file classes?

@@ +329,5 @@
> +}
> +
> +// Attempt to parse an int64 literal optionally surrounded by whitespace,
> +// returns true on success, false otherwise.
> +static bool safe_strtoi64(const char *s, int64_t *val) {

There's a whole bunch of number parsing stuff in ./content/base/src/nsAttrValue.cpp. Can we use any of that? We also have ToInteger and stuff on our string classes. Is there anything int64 related we can do there?

@@ +342,5 @@
> +        return false;
> +    }
> +
> +    // Skip trailing whitespace
> +    while (isspace(*end)) {

Do you need to check if end > s+strlen(s)? Or pass the number of bytes in s to safe_strtoi64 to check?

@@ +450,5 @@
> +
> +status_t GonkRecorder::setParamMaxFileDurationUs(int64_t timeUs) {
> +    LOGV("setParamMaxFileDurationUs: %lld us", timeUs);
> +
> +    // This is meant for backward compatibility for MediaRecorder.java

Can you go into more detail here?

@@ +459,5 @@
> +        LOGE("Max file duration is too short: %lld us", timeUs);
> +        return BAD_VALUE;
> +    }
> +
> +    if (timeUs <= 15 * 1000000LL) {

Why was this magic number chosen?

@@ +480,5 @@
> +        return BAD_VALUE;
> +    }
> +
> +    if (bytes <= 100 * 1024) {
> +        LOGW("Target file size (%lld bytes) is too small to be respected", bytes);

Why was this magic number chosen?

@@ +495,5 @@
> +
> +status_t GonkRecorder::setParamInterleaveDuration(int32_t durationUs) {
> +    LOGV("setParamInterleaveDuration: %d", durationUs);
> +    if (durationUs <= 500000) {           //  500 ms
> +        // If interleave duration is too small, it is very inefficient to do

Why was this number chosen?

@@ +500,5 @@
> +        // interleaving since the metadata overhead will count for a significant
> +        // portion of the saved contents
> +        LOGE("Audio/video interleave duration is too small: %d us", durationUs);
> +        return BAD_VALUE;
> +    } else if (durationUs >= 10000000) {  // 10 seconds

Why was this number chosen?

@@ +570,5 @@
> +
> +    // The range is set to be the same as the audio's time scale range
> +    // since audio's time scale has a wider range.
> +    if (timeScale < 600 || timeScale > 96000) {
> +        LOGE("Time scale (%d) for movie is out of range [600, 96000]", timeScale);

Why were these numbers chosen?

@@ +583,5 @@
> +
> +    // 60000 is chosen to make sure that each video frame from a 60-fps
> +    // video has 1000 ticks.
> +    if (timeScale < 600 || timeScale > 60000) {
> +        LOGE("Time scale (%d) for video is out of range [600, 60000]", timeScale);

So for the rest of this file where you are checking magic numbers, can you explain with comments why these numbers were picked.

@@ +656,5 @@
> +    LOGV("setParameter: key (%s) => value (%s)", key.string(), value.string());
> +    if (key == "max-duration") {
> +        int64_t max_duration_ms;
> +        if (safe_strtoi64(value.string(), &max_duration_ms)) {
> +            return setParamMaxFileDurationUs(1000LL * max_duration_ms);

Need to check for overflow here?

@@ +990,5 @@
> +        OMXCodec::Create(client.interface(), encMeta,
> +                         true /* createEncoder */, audioSource);
> +#else
> +    // use direct OMX interface instead of connecting to
> +    // mediaserver over binder calls

Explain why this is the case. ie. Why use direct OMX vs OMXClient.

@@ +1099,5 @@
> +status_t GonkRecorder::startFMA2DPWriter() {
> +    /* FM soc outputs at 48k */
> +	mSampleRate = 48000;
> +	mAudioChannels = 2;
> +	

Trim leading whitespace.

@@ +1596,5 @@
> +    CHECK(meta->findInt32(kKeyColorFormat, &colorFormat));
> +    CHECK(meta->findInt32(kKeyHFR, &hfr));
> +
> +    if(hfr) {
> +      mMaxFileDurationUs = mMaxFileDurationUs * (hfr/mFrameRate);

Can frame rate be zero? If so, check for divide by zero. Can this multiplication overflow? If so, handle.

@@ +1614,5 @@
> +
> +    char mDeviceName[100];
> +    property_get("ro.board.platform",mDeviceName,"0");
> +    if(!strncmp(mDeviceName, "msm7627a", 8)) {
> +      if(hfr && (width * height > 432*240)) {

Can "width * height" overflow and fall below 432*240 as a result? If so, check and handle.

@@ +1620,5 @@
> +        return INVALID_OPERATION;
> +      }
> +    }
> +    else {
> +      if(hfr && ((mVideoEncoder != VIDEO_ENCODER_H264) || (width * height > 800*480))) {

Overflow check as above.

::: dom/camera/GonkRecorder.h
@@ +128,5 @@
> +    int32_t mRotationDegrees;  // Clockwise
> +    int32_t mLatitudex10000;
> +    int32_t mLongitudex10000;
> +    int32_t mStartTimeOffsetMs;
> +

Document all these members. What they're for, what units they are.

::: dom/camera/nsIDOMCameraManager.idl
@@ +178,1 @@
>  };

Need to rev UUID with interface change?

Comment 11

[Andrew Overholt [:overholt]]

Video recording is a requirement for v1 AFAIK.

Comment 12

[Inder]

(In reply to Chris Double (:doublec) from comment #10)
> Comment on attachment 654324 [details] [diff] [review]
> WIP - video recording support
> 
> Review of attachment 654324 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You'll need to split your patch up so you can get review from the right
> parties. configure.in will need a build peer for example. I'm not a DOM peer
> myself so I'm not sure I'm the best person to be reviewing this as I don't
> see much in the way of video/audio media code but I'll do a preliminary
> review anyway. 

Sure. MikeH can you please split the patch in following parts:
1) build changes
2) new code derived from android source, the files are listed in the following response.
3) changes to camera code

> 
> Do some files come from Android source? I see their copyright header. If you
> could put these in a separate patch and provide some way of updating them to
> latest Android source, or showing the differences between them and the
> original Android version. The media/omx-plugin code provides a shell script
> and patch file for example.

Actually in the android derived code in this patch I have used "#if 0..#else.. #endif" block. These are the places I had to make b2g specific changes and code in #else block is b2g specific. Everything else in these files is android code as is.
GonkCameraSource.cpp/.h <-- StagefrightRecorder.cpp/.h
GonkRecorder.cpp/.h <-- CameraSource.cpp/.h
AudioParameter.cpp <-- taken as is.
GonkCameraListener.h <-- comes from frameworks/base/include/camera/Camera.h 

> 
> I did not look at any of the "#if 0" blocks of code. I assume these will be
> removed rather than enabled.

This was intentionally kept to show how the modifications were done in the android code for b2g and will also help in future upkeep of the code.

> 
> ::: dom/camera/GonkCameraControl.cpp
> @@ +614,5 @@
> >  
> > +nsresult
> > +nsGonkCameraControl::SetupVideoMode()
> > +{
> > +  // read perferences for video mode
> 
> Spelling of 'perferences'

Will fix.

> 
> @@ +634,5 @@
> > +
> > +  PullParametersImpl(nullptr);
> > +
> > +  // set params with new values
> > +  const size_t SIZE = 256;
> 
> Why was the magic number '256' chosen? Add a comment.

Needed a large enough buffer to construct and set param. Will add a comment.

> 
> @@ +694,5 @@
> > +  }
> > +  return rv;
> > +}
> > +
> > +static int
> 
> Add a comment to describe what the return value is.
> 
> @@ +695,5 @@
> > +  return rv;
> > +}

This should be NS_OK actually. Will fix.

> > +
> > +static int
> > +GetFileNameWithDate(char* outstr, int maxlen)
> 
> I'm not familiar with the style guide requirements for DOM code but
> everywhere else I've done stuff for Gecko parameter names need to be like
> aOutstr, aMaxlen, etc. This goes for all the parameter names used in this
> and other files.

Will fix in the original b2g code but is probably not a good idea for android derived code specially for future upkeep of that code.

> 
> @@ +703,5 @@
> > +  int actualLen;
> > +
> > +  t = time(NULL);
> > +  tmp = localtime(&t);
> > +  if (tmp == NULL) {
> 
> Restructure to declare variable and assign at the same time as is the idiom
> for C++. eg:
> 
> time_t = time(NULL);
> struct tm* tmp = localtime(&t);
> etc
> 

Sure, will fix.

> @@ +725,5 @@
> > +  }
> > +  return ".3gp";
> > +}
> > +
> > +static int
> 
> Comment documenting what the return value is.

Sure, will fix.

> 
> @@ +734,5 @@
> > +  char fileName[MAX_VIDEO_FILE_NAME_LEN];
> > +  char fileName2[MAX_VIDEO_FILE_NAME_LEN];
> > +  char* videoFileAbsPath;
> > +  struct stat buffer;
> > +  int result;
> 
> Declare the variables at the point you first use them rather than the C
> approach of declaring them first at the top of the function.

Sure, will fix.

> 
> @@ +742,5 @@
> > +    strncpy(fileName, DEFAULT_VIDEO_FILE_NAME, MAX_VIDEO_FILE_NAME_LEN);
> > +  }
> > +
> > +  // add the extension
> > +  strncat(fileName, GetFileExtension(nVideoFileFormat), MAX_VIDEO_FILE_NAME_LEN);
> 
> If the result of strlen(GetFileExtension(nVideoFileFormat)) +
> strlen(fileName) > MAX_VIDEO_FILE_NAME_LEN then this will write out of
> bounds of fileName. The third parameter bounds the size of the second, not
> the first.
> 
> Ideally you should use Mozilla's string handling APIs to avoid these issues
> rather than C functions.

Good catch, will fix.

> 
> @@ +747,5 @@
> > +
> > +  DOM_CAMERA_LOGI("Video File Name: \"%s\" \n", fileName);
> > +
> > +  // check if the video storage dir exists
> > +  result = stat(VIDEO_STORAGE_DIR, &buffer);
> 
> Why use 'stat' and not nsIFile routines?

See comment below about why we need fd.

> 
> @@ +774,5 @@
> > +#endif
> > +  }
> > +
> > +  DOM_CAMERA_LOGI("Opening video file: %s\n", videoFileAbsPath);
> > +  int fd = open(videoFileAbsPath, O_RDWR | O_CREAT, 0744);
> 
> Why use C routines like 'open' instead of equivalent Mozilla API's (nsIFile,
> etc)?

See comment below why we need fd.

> 
> @@ +796,5 @@
> > +
> > +nsresult
> > +nsGonkCameraControl::SetupRecording()
> > +{
> > +  const size_t SIZE = 256;
> 
> Comment explaining why this magic value was picked.

Again, just needed a big enough buffer to construct and set the params. Will add a comment here.

> 
> @@ +831,5 @@
> > +  int fd = CreateVideoFile(mVideoFile, mVideoFileFormat);
> > +  if (fd < 0) {
> > +    return NS_ERROR_FAILURE;
> > +  }
> > +  CHECK_SETARG(mRecorder->setOutputFile(fd, 0, 0));
> 
> Why are file descriptors used instead of Mozilla file api functions? Is it a
> requirement of interfacing to the Android code?
> 

That is correct, it is a requirement in this case. We need to have a file descriptor and none of the mozilla functions provides that so had to use c function to get fd. Discussed with Fabrice about it earlier and he was OK with it.

> ::: dom/camera/GonkCameraControl.h
> @@ +77,5 @@
> > +  PRUint32                  mVideoWidth;
> > +  PRUint32                  mVideoHeight;
> > +  nsString                  mVideoFile;
> > +
> > +  // video mode settings
> 
> Add comments for each of these additional members. What unit is 'mDuration'.
> What do the values of 'mVideoFileFormat' mean, etc.

Sure, will do.

> 
> ::: dom/camera/GonkCameraHwMgr.cpp
> @@ +164,5 @@
> >  void
> > +GonkCameraHardware::DataCallbackTimestamp(nsecs_t timestamp, int32_t aMsgType, const sp<IMemory> &aDataPtr, void* aUser)
> > +{
> > +  DOM_CAMERA_LOGI("%s",__func__);
> > +  GonkCameraHardware* hw = GetHardware((PRUint32)aUser);
> 
> Use reinterpret_cast here and other places you do this cast.

Hmm.. seems like this needs to be fixed even in original code and not just in this patch?

> 
> @@ +499,5 @@
> > +int
> > +GonkCameraHardware::StartRecording(PRUint32 aHwHandle)
> > +{
> > +  DOM_CAMERA_LOGI("%s: aHwHandle = %d\n", __func__, aHwHandle);
> > +  int rv = OK;
> 
> s/int/status_t/ I think.

No, this needs to be int here as it's using android return types.

> 
> ::: dom/camera/GonkCameraHwMgr.h
> @@ +48,4 @@
> >    ~GonkCameraHardware();
> >    void init();
> >  
> > +  static void                   DataCallbackTimestamp(nsecs_t timestamp, int32_t aMsgType, const sp<IMemory>& aDataPtr, void* aUser);

Sure, will fix.

> 
> s/timestamp/aTimestamp/
> 
> ::: dom/camera/GonkCameraListener.h
> @@ +11,5 @@
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> 
> I notice this is new code but uses the Android license not the MPL. I don't
> know what the licensing requirements are for gecko code for this sort of
> thing but thought I should point it out.

This was actually derived from frameworks/base/include/camera/Camera.h so kept original android license.

> 
> @@ +25,5 @@
> > +// ref-counted object for callbacks
> > +class GonkCameraListener: virtual public RefBase
> > +{
> > +public:
> > +    virtual void notify(int32_t msgType, int32_t ext1, int32_t ext2) = 0;
> 
> aMsgType, aExt1, etc.
> 
> ::: dom/camera/GonkCameraPreview.cpp
> @@ +26,4 @@
> >  
> >  using namespace mozilla;
> >  
> > +#define YUV_CONVERT_INPLACE   0
> 
> Comments explaining what this does and what values it can have.

Actually, this is stale code which needed to be removed. Will fix.

> 
> ::: dom/camera/GonkCameraSource.cpp
> @@ +76,5 @@
> > +    : mSource(source) {
> > +}
> > +
> > +GonkCameraSourceListener::~GonkCameraSourceListener() {
> > +}
> 
> Other code in this patch puts the opening brace for a function on the
> following line. Please do that here, or change all the other usage.

Sure, will fix.

> 
> @@ +103,5 @@
> > +    }
> > +}
> > +
> > +static int32_t getColorFormat(const char* colorFormat) {
> > +    return OMX_COLOR_FormatYUV420SemiPlanar;
> 
> You have dead code following this return.

True, this is Android code as it is and didn't change anything here. Don't think we need to fix.

> 
> @@ +279,5 @@
> > +
> > +/*
> > + * If the preview and video output is separate, we only set the
> > + * the video size, and applications should set the preview size
> > + * to some proper value, and the recording framework will not
> 
> "...set the the video size..." <-- duplicated "the"

Same comment as above.

> 
> @@ +299,5 @@
> > +static void getSupportedVideoSizes(
> > +    const CameraParameters& params,
> > +    bool *isSetVideoSizeSupported,
> > +    Vector<Size>& sizes) {
> > +
> 
> Why is the isSetVideoSizeSupported a pointer to a boolean? Why not make it
> the return value of the function?
> 
> @@ +376,5 @@
> > +        const char* supportedFrameRates =
> > +                params->get(CameraParameters::KEY_SUPPORTED_PREVIEW_FRAME_RATES);
> > +        CHECK(supportedFrameRates != NULL);
> > +        LOGV("Supported frame rates: %s", supportedFrameRates);
> > +        char buf[4];
> 
> Why the magic value '4' here?

Seems like it just needs a big enough buffer to store frame rate.

> 
> @@ +401,5 @@
> > +            LOGE("Could not change settings."
> > +                 " Someone else is using camera %p?", mCamera.get());
> > +#else
> > +        if (OK != GonkCameraHardware::PushParameters(mCameraHandle,*params)) {
> > +            LOGE("Could not change settings."
> 
> In some places in this patch you use reverse conditional checks
> (some_constant == some_var) instead of (some_var == some_constant). In other
> places you didn't. Please standardise on one usage. I'd prefer the latter.
> 
Sure, will fix.

> @@ +622,5 @@
> > +      LOGW("Invalid hfr value(%d) set from app. Disabling HFR.", hfr);
> > +      hfr = 0;
> > +    }
> > +
> > +    int64_t glitchDurationUs = (1000000LL / mVideoFrameRate);
> 
> Can mVideoFrameRate be zero here? If so you'll want to handle the divide by
> zero case.

No, looking at the logic it will never go to 0.

> 
> @@ +742,5 @@
> > +            mCamera->disconnect();
> > +        }
> > +        mCamera->unlock();
> > +        mCamera.clear();
> > +        mCamera = 0;
> 
> Should this '0' be nsnullptr?

This is in #if 0 block so will never get executed. This was android code that we didn't need in b2g.

> 
> @@ +771,5 @@
> > +    releaseQueuedFrames();
> > +    while (!mFramesBeingEncoded.empty()) {
> > +        if (NO_ERROR !=
> > +            mFrameCompleteCondition.waitRelative(mLock,
> > +                    mTimeBetweenFrameCaptureUs * 1000LL + CAMERA_SOURCE_TIMEOUT_NS)) {
> 
> Can this calculation overflow? If so you should check and handle it. It
> probably depends if mTimeBetweenFrameCaptureUs is bounded previously.

mTimeBetweenFrameCaptureUs is currently not being used so we should be good.

> 
> ::: dom/camera/GonkCameraSource.h
> @@ +148,5 @@
> > +        virtual void binderDied(const wp<IBinder>& who);
> > +    };
> > +#endif
> > +
> > +    enum CameraFlags {
> 
> Document what these flags mean.

Original android code and not being used in b2g so will comment it out.

> 
> @@ +158,5 @@
> > +    Size     mVideoSize;
> > +    int32_t  mVideoFrameRate;
> > +    int32_t  mColorFormat;
> > +    status_t mInitCheck;
> > +
> 
> Comments explaining what these members are and what units they are in. Same
> with all the other members in the class that aren't documented.

Sure, can add comment.


> 
> @@ +174,5 @@
> > +    bool mStarted;
> > +    int32_t mNumFramesEncoded;
> > +
> > +    // Time between capture of two frames.
> > +    int64_t mTimeBetweenFrameCaptureUs;
> 
> What unit is this? Seconds, milliseconds, etc?

Microseconds. I thought it was clear from the variable name (Us) but will update the comment here.

> 
> ::: dom/camera/GonkRecorder.cpp
> @@ +98,5 @@
> > +}
> > +#endif
> > +
> > +#if 1
> > +static sp<IOMX> sOMX = NULL;
> 
> This is the same as what's in the media plugin code and I believe Edwin's
> "optimized stagefright" patch has it also. How many static OMX's are we
> going to have? Should this be factored out so that one is shared? Actually
> that's what I thought OMXClient was for. Can we use that?

If the GetOMX code is factored out from OmxPlugin code then this can probably use it. We need to get a direct OMX interface instead of one talking to mediaserver over binder calls do definitely need to do "new OMX()".

> 
> @@ +217,5 @@
> > +    }
> > +
> > +    // Use default values if appropriate setparam's weren't called.
> > +    if(mAudioEncoder == AUDIO_ENCODER_AAC) {
> > +        mSampleRate = mSampleRate ? mSampleRate : 48000;
> 
> Explain why these default values were chosen.

Not 100% sure but this is the original android code and the values seems like the standard values for that encoding format.

> 
> @@ +221,5 @@
> > +        mSampleRate = mSampleRate ? mSampleRate : 48000;
> > +        mAudioChannels = mAudioChannels ? mAudioChannels : 2;
> > +        mAudioBitRate = mAudioBitRate ? mAudioBitRate : 156000;
> > +    }
> > +    else{
> 
> Explain why these default values were chosen.

See above comment.

> 
> @@ +322,5 @@
> > +
> > +    if (mOutputFd >= 0) {
> > +        ::close(mOutputFd);
> > +    }
> > +    mOutputFd = dup(fd);
> 
> Why are we using file descriptors and not our file classes?

It's because MPEG4Writer in stagefright needs to have a fd.

> 
> @@ +329,5 @@
> > +}
> > +
> > +// Attempt to parse an int64 literal optionally surrounded by whitespace,
> > +// returns true on success, false otherwise.
> > +static bool safe_strtoi64(const char *s, int64_t *val) {
> 
> There's a whole bunch of number parsing stuff in
> ./content/base/src/nsAttrValue.cpp. Can we use any of that? We also have
> ToInteger and stuff on our string classes. Is there anything int64 related
> we can do there?

This is the original android code and since it's working fine here I am not sure if we need to change it. Will probably be a pain for future upkeep.

> 
> @@ +342,5 @@
> > +        return false;
> > +    }
> > +
> > +    // Skip trailing whitespace
> > +    while (isspace(*end)) {
> 
> Do you need to check if end > s+strlen(s)? Or pass the number of bytes in s
> to safe_strtoi64 to check?
> 

Probably.

> @@ +450,5 @@
> > +
> > +status_t GonkRecorder::setParamMaxFileDurationUs(int64_t timeUs) {
> > +    LOGV("setParamMaxFileDurationUs: %lld us", timeUs);
> > +
> > +    // This is meant for backward compatibility for MediaRecorder.java
> 
> Can you go into more detail here?

Original android comment and not relevant to us but the value checks looks relevant and appropriate.

> 
> @@ +459,5 @@
> > +        LOGE("Max file duration is too short: %lld us", timeUs);
> > +        return BAD_VALUE;
> > +    }
> > +
> > +    if (timeUs <= 15 * 1000000LL) {
> 
> Why was this magic number chosen?

Original android code. Since I didn't write this code I am not sure why these values were chosen. In this case it seems like it didn't want max duration to be too small.

> 
> @@ +480,5 @@
> > +        return BAD_VALUE;
> > +    }
> > +
> > +    if (bytes <= 100 * 1024) {
> > +        LOGW("Target file size (%lld bytes) is too small to be respected", bytes);
> 
> Why was this magic number chosen?

Similar thing as above.

> 
> @@ +495,5 @@
> > +
> > +status_t GonkRecorder::setParamInterleaveDuration(int32_t durationUs) {
> > +    LOGV("setParamInterleaveDuration: %d", durationUs);
> > +    if (durationUs <= 500000) {           //  500 ms
> > +        // If interleave duration is too small, it is very inefficient to do
> 
> Why was this number chosen?

In this case the comment near that line explains why those values were chosen. Explanation not sufficient?

> 
> @@ +500,5 @@
> > +        // interleaving since the metadata overhead will count for a significant
> > +        // portion of the saved contents
> > +        LOGE("Audio/video interleave duration is too small: %d us", durationUs);
> > +        return BAD_VALUE;
> > +    } else if (durationUs >= 10000000) {  // 10 seconds
> 
> Why was this number chosen?

See previous comment.

> 
> @@ +570,5 @@
> > +
> > +    // The range is set to be the same as the audio's time scale range
> > +    // since audio's time scale has a wider range.
> > +    if (timeScale < 600 || timeScale > 96000) {
> > +        LOGE("Time scale (%d) for movie is out of range [600, 96000]", timeScale);
> 
> Why were these numbers chosen?

Original android code. Since I didn't write this code I am not sure why these values were chosen.

> 
> @@ +583,5 @@
> > +
> > +    // 60000 is chosen to make sure that each video frame from a 60-fps
> > +    // video has 1000 ticks.
> > +    if (timeScale < 600 || timeScale > 60000) {
> > +        LOGE("Time scale (%d) for video is out of range [600, 60000]", timeScale);
> 
> So for the rest of this file where you are checking magic numbers, can you
> explain with comments why these numbers were picked.

See above comment.

> 
> @@ +656,5 @@
> > +    LOGV("setParameter: key (%s) => value (%s)", key.string(), value.string());
> > +    if (key == "max-duration") {
> > +        int64_t max_duration_ms;
> > +        if (safe_strtoi64(value.string(), &max_duration_ms)) {
> > +            return setParamMaxFileDurationUs(1000LL * max_duration_ms);
> 
> Need to check for overflow here?

Probably.

> 
> @@ +990,5 @@
> > +        OMXCodec::Create(client.interface(), encMeta,
> > +                         true /* createEncoder */, audioSource);
> > +#else
> > +    // use direct OMX interface instead of connecting to
> > +    // mediaserver over binder calls
> 
> Explain why this is the case. ie. Why use direct OMX vs OMXClient.

Explained in one of the comments above.

> 
> @@ +1099,5 @@
> > +status_t GonkRecorder::startFMA2DPWriter() {
> > +    /* FM soc outputs at 48k */
> > +	mSampleRate = 48000;
> > +	mAudioChannels = 2;
> > +	
> 
> Trim leading whitespace.
> 
> @@ +1596,5 @@
> > +    CHECK(meta->findInt32(kKeyColorFormat, &colorFormat));
> > +    CHECK(meta->findInt32(kKeyHFR, &hfr));
> > +
> > +    if(hfr) {
> > +      mMaxFileDurationUs = mMaxFileDurationUs * (hfr/mFrameRate);
> 
> Can frame rate be zero? If so, check for divide by zero. Can this
> multiplication overflow? If so, handle.
> 
> @@ +1614,5 @@
> > +
> > +    char mDeviceName[100];
> > +    property_get("ro.board.platform",mDeviceName,"0");
> > +    if(!strncmp(mDeviceName, "msm7627a", 8)) {
> > +      if(hfr && (width * height > 432*240)) {
> 
> Can "width * height" overflow and fall below 432*240 as a result? If so,
> check and handle.
> 
> @@ +1620,5 @@
> > +        return INVALID_OPERATION;
> > +      }
> > +    }
> > +    else {
> > +      if(hfr && ((mVideoEncoder != VIDEO_ENCODER_H264) || (width * height > 800*480))) {
> 
> Overflow check as above.
> 
> ::: dom/camera/GonkRecorder.h
> @@ +128,5 @@
> > +    int32_t mRotationDegrees;  // Clockwise
> > +    int32_t mLatitudex10000;
> > +    int32_t mLongitudex10000;
> > +    int32_t mStartTimeOffsetMs;
> > +
> 
> Document all these members. What they're for, what units they are.

This is the original android code but will try to add some comments.

> 
> ::: dom/camera/nsIDOMCameraManager.idl
> @@ +178,1 @@
> >  };
> 
> Need to rev UUID with interface change?

Probably, mikeh?

Comment 13

[Dave Hylands [:dhylands]]

(In reply to Inder from comment #12)
> (In reply to Chris Double (:doublec) from comment #10)
> > Do some files come from Android source? I see their copyright header. If you
> > could put these in a separate patch and provide some way of updating them to
> > latest Android source, or showing the differences between them and the
> > original Android version. The media/omx-plugin code provides a shell script
> > and patch file for example.
> 
> Actually in the android derived code in this patch I have used "#if
> 0..#else.. #endif" block. These are the places I had to make b2g specific
> changes and code in #else block is b2g specific. Everything else in these
> files is android code as is.

You may want to do something like

#define USE_ORIGINAL_ANDROID_CODE 0

and use

#if USE_ORIGINAL_ANDROID_CODE

which is much more explanatory than just #if 0

Comment 14

[Inder]

(In reply to Dave Hylands [:dhylands] from comment #13)
> (In reply to Inder from comment #12)
> > (In reply to Chris Double (:doublec) from comment #10)
> > > Do some files come from Android source? I see their copyright header. If you
> > > could put these in a separate patch and provide some way of updating them to
> > > latest Android source, or showing the differences between them and the
> > > original Android version. The media/omx-plugin code provides a shell script
> > > and patch file for example.
> > 
> > Actually in the android derived code in this patch I have used "#if
> > 0..#else.. #endif" block. These are the places I had to make b2g specific
> > changes and code in #else block is b2g specific. Everything else in these
> > files is android code as is.
> 
> You may want to do something like
> 
> #define USE_ORIGINAL_ANDROID_CODE 0
> 
> and use
> 
> #if USE_ORIGINAL_ANDROID_CODE
> 
> which is much more explanatory than just #if 0

Sure, I had a macro defined earlier but later decided to take it out. 
Since majority of the code is Android and there are few modifications for B2G I will define something like:
#define USE_B2G_SPECIFIC_MODIFICATIONS 1
and change the if-def blocks accordingly.

Comment 15

[cajbir (:cajbir)]

The review would have been a lot easier if there was some way of identifying "this is original android code". You are right that there is no point changing that code (unless there is a bug) to ease maintenance and later merging. I much prefer the approach the media plugin code (and other media libraries) use of including a diff against the original android file and your changes in the tree so pulling in android updates is made easier.

I'd remove all the "#if 0", add a .patch file containing a diff against the original android file. Include a README explaining where the original file was from, what git/svn version it is from, and a shell script to apply the patches to the original to produce what's in our tree.

Comment 16

[Dietrich Ayala (:dietrich)]

Hey y'all, been about a week without changes here. Inder, do you have an ETA for a new patch?

Comment 17

[Inder]

(In reply to Dietrich Ayala (:dietrich) from comment #16)
> Hey y'all, been about a week without changes here. Inder, do you have an ETA
> for a new patch?
Dietrich, The current patch has MPL code so I can't update the patch as it is. MikeH is rebasing and breaking it into various parts before i can update the android inherited code as suggested by Chris Double.

Comment 18

[Dietrich Ayala (:dietrich)]

Thanks Inder. Mike, are you going to be able to break this code up soon?

Comment 19

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

(In reply to Dietrich Ayala (:dietrich) from comment #18)
> Thanks Inder. Mike, are you going to be able to break this code up soon?

Hi Dietrich, I'm working on it now.  Just finishing the merge and rebase with the changes introduced in the fix to bug 779139, and am tracking down a SIGSEGV that appears when the video camera is closed.

If I can't track it down by tomorrow morning, I'll just push the patch fragments as-is and work on the error in parallel.  Unless Inder has cycles to do that... :)

Comment 20

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

Update: I'm fighting a new issue that seems to have started since bug 779139 landed on m-c: see bug 789090.  This issue is going to be delayed until that issue is resolved.

Comment 21

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

Attachment: WIP - video recording support

Inder, I need you to take a look at this rebased/updated version of your patch.  The recording preview starts correctly, but recording fails to start.  I don't see anything obvious in the log.

Comment 22

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

Attachment: camera.js that exercises the recording API

Comment 23

[Inder]

(In reply to Mike Habicher [:mikeh] from comment #21)
> Created attachment 659610 [details] [diff] [review]
> WIP - video recording support
> 
> Inder, I need you to take a look at this rebased/updated version of your
> patch.  The recording preview starts correctly, but recording fails to
> start.  I don't see anything obvious in the log.

Mike,
  As I mentioned over the email after looking at the log that there seems to be some issue with the recording frame the GonkCameraSource.cpp is receiving in GonkCameraSource::read() function. The message "Timed out waiting for incoming camera video..." is coming from that function. You will need to add some logs to figure out what's going wrong with the frames.

Comment 24

[Inder]

MikeH, While you debug the recording issue can you please break the patch as we discussed so review can proceed.

Chris, You mentioned in comment 15 that "I much prefer the approach the media plugin code (and other media libraries) use of including a diff against the original android file and your changes in the tree so pulling in android updates is made easier."
Can you please point me to the patch file and the shell script media plugin code used so I can try to do something similar. Does these files reside with the implementation or in the bug?

Comment 25

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

(In reply to Inder from comment #23)
>
>   As I mentioned over the email after looking at the log that there seems to
> be some issue with the recording frame the GonkCameraSource.cpp is receiving
> in GonkCameraSource::read() function. The message "Timed out waiting for
> incoming camera video..." is coming from that function. You will need to add
> some logs to figure out what's going wrong with the frames.

I've added some more instrumentation to the log, and here's what I see:
1. the call to postDataTimestamp() returns before the stall;
2. the read() function, which is waiting, is signalled and resumes;
3. read() moves the incoming buffer from mFramesReceived to mFramesBeginEncoded;
4. read() exits;
5. read() is called again, and hits the waitRelative() call, and stays stuck inside that while loop indefinitely;
6. no new calls to postDataTimestamp() take place.

Right now, it looks like something has gone wrong in the camera code.  Inder, you're the expert here--I need you to take a look.

Comment 26

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

Attachment: WIP - video recording support (20120912-1228)

Latest patch, including fix for stall on recording.

Broken up patches coming soon.

Comment 27

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

Attachment: Part 0: build changes

Comment 28

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

Attachment: Part 1: media changes

Comment 29

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

Attachment: Part 2: camera framework changes

Comment 30

[Inder]

Attachment: Part 1: new media changes with README, update.patch and update.sh

Chris, as per your request removed all "#if 0" and added the following files:
README      : explains where the android files were taken from.
update.patch: shows my changes on top of the android files.
update.sh   : shell script to apply the update.patch to the original android files.
Hopefully, this will simplify the review. Thanks for your feedback.

MikeH, removed the pieces to enable 3GPP support as it's being tracked by a separate bug (779209). Can you please obsolete the previous "Part 1" patch and update the "WIP - video recording support" patch with this one.

Comment 31

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

Comment on attachment 660545 [details] [diff] [review]
Part 1: media changes

Marking previous 'Part 1' patch obselete as requested.

Comment 32

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

Comment on attachment 662612 [details] [diff] [review]
Part 1: new media changes with README, update.patch and update.sh

Tag this as a patch.

Comment 33

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

Comment on attachment 660542 [details] [diff] [review]
WIP - video recording support (20120912-1228)

Give the patch a unique identifier to help with inter-patch diffing.

Comment 34

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

Attachment: WIP - video recording support (20120920-1235)

Update WIP patch with latest changes, as requested.

In the future, please make sure the 'Part 1' patch is ref'd to gecko, and not gecko/dom/camera.  It makes the merging easier.

This patch also removes the changes to:
- netwerk/mime/nsMimeTypes.h
- uriloader/exthandler/nsExternalHelperAppService.cpp
...as those changes are tracked by bug 779209, which you will also need to apply.

Comment 35

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

Comment on attachment 663116 [details] [diff] [review]
WIP - video recording support (20120920-1235)

Forgot to set the correct version ID.

Comment 36

[Dale Harvey (:daleharvey)]

Hey mikeh

I have tried applying this patch and testing on the Otoro, I am seeing a failure in 

    startRecording(function onsuccess() { 
    }, function onerror(e) { 
      console.log(e);
    });

I get 'FAILURE' printed at the log, with no other indication of the problem in logcat. Any ideas?

Comment 37

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

(In reply to [e-mail] from comment #36)
> Hey mikeh
> 
> I have tried applying this patch and testing on the Otoro, I am seeing a
> failure in 
> 
>     startRecording(function onsuccess() { 
>     }, function onerror(e) { 
>       console.log(e);
>     });
> 
> I get 'FAILURE' printed at the log, with no other indication of the problem
> in logcat. Any ideas?

If you can post a logcat--even a snippet--that will help in narrowing down what's going on here.

Inder, would you like me to review Part 0 and Part 2?

Comment 38

[Inder]

> Inder, would you like me to review Part 0 and Part 2?
MikeH, sure.

Comment 39

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

Comment on attachment 660546 [details] [diff] [review]
Part 2: camera framework changes

Hmm, I probably shouldn't be the official reviewer of this file, since I made quite a few of the changes during integration.  Anyone else have some cycles to spare?

Comment 40

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

Comment on attachment 660546 [details] [diff] [review]
Part 2: camera framework changes

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

Review of the code I wasn't personally involved in writing/merging.

::: dom/camera/GonkCameraControl.cpp
@@ +880,5 @@
> +
> +  // set params with new values
> +  const size_t SIZE = 256;
> +  char buffer[SIZE];
> +  // TODO: Ignore the width and height settings from app, just use the one in profile

Please create a new bug to track this TODO.

@@ +885,5 @@
> +  // eventually, will try to choose a profile which respects the settings from app
> +  mParams.setPreviewSize(mVideoFrameWidth, mVideoFrameHeight);
> +  mParams.setPreviewFrameRate(mVideoFrameRate);
> +  snprintf(buffer, SIZE, "%dx%d", mVideoFrameWidth, mVideoFrameHeight);
> +  // TODO: "record-size" is probably deprecated in later ICS

Please create a new bug to track this TODO.

@@ +924,5 @@
> +{
> +  if (nVideoFileFormat == OUTPUT_FORMAT_MPEG_4) {
> +    return ".mp4";
> +  }
> +  return ".3gp";

Is nVideoFileFormat limited to OUTPUT_FORMAT_MPEG_4 and (what I assume is) OUTPUT_FORMAT_3GPP?  If not, this should be changed into a switch-case with a default case that (perhaps) returns nullptr.

@@ +930,5 @@
> +
> +static int
> +CreateVideoFile(nsAString& aVideoFile, int nVideoFileFormat)
> +{
> +  nsCOMPtr<nsIFile> f;

Move declaration of 'f' down to where it is first used.

@@ +951,5 @@
> +
> +  // check if the video storage dir exists
> +  result = stat(VIDEO_STORAGE_DIR, &buffer);
> +  if (0 == result) {
> +    snprintf(fileName2, MAX_VIDEO_FILE_NAME_LEN, VIDEO_STORAGE_DIR"/%s", fileName);

Do we want to check snprintf() for problematic return values?

@@ +956,5 @@
> +    videoFileAbsPath = fileName2;
> +  } else {
> +    DOM_CAMERA_LOGW("%s stat failed with error: %d:%s\n", VIDEO_STORAGE_DIR, errno, strerror(errno));
> +#if DEFAULT_VIDEO_STORAGE_TO_TEMP_DIR
> +    nsAutoCString filePath;

Move declaration of 'filePath' down to where it is first used.

@@ +964,5 @@
> +    if (!f) {
> +      DOM_CAMERA_LOGE("Failed to get temp directory path\n");
> +      return -1;
> +    }
> +    if (NS_FAILED(f->AppendNative(nsDependentCString(fileName)))) {

Don't make function call inside a macro.  Break this out as |nsresult rv = ...; if (NS_FAILED(rv)) {...|.

@@ +987,5 @@
> +}
> +
> +#ifndef CHECK_SETARG
> +#define CHECK_SETARG(x)                 \
> +  {                                     \

A style nit: wrapping this macro in do-while will prevent it from causing problems should it ever get included in a wrapping if, e.g.:

  do { \
    /* multi-line macro body here */ \
  while(0)

@@ +1021,5 @@
> +  CHECK_SETARG(mRecorder->setParameters(String8(buffer)));
> +  snprintf(buffer, SIZE, "audio-param-sampling-rate=%d", mAudioSampleRate);
> +  CHECK_SETARG(mRecorder->setParameters(String8(buffer)));
> +  CHECK_SETARG(mRecorder->setAudioEncoder((audio_encoder)mAudioCodec));
> +  // TODO: For now there is no limit on recording duration

Please file a new bug to track this TODO.

@@ +1023,5 @@
> +  CHECK_SETARG(mRecorder->setParameters(String8(buffer)));
> +  CHECK_SETARG(mRecorder->setAudioEncoder((audio_encoder)mAudioCodec));
> +  // TODO: For now there is no limit on recording duration
> +  CHECK_SETARG(mRecorder->setParameters(String8("max-duration=-1")));
> +  // TODO: For now there is no limit on file size

Please file a new bug to track this TODO.

Comment 41

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

Comment on attachment 660546 [details] [diff] [review]
Part 2: camera framework changes

:cjones, can you review part 2?

Comment 42

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Mike Habicher [:mikeh] from comment #41)
> Comment on attachment 660546 [details] [diff] [review]
> Part 2: camera framework changes
> 
> :cjones, can you review part 2?

I can pinch-review this code if needed, but I don't understand it very well.

Kan-Ru, do you feel comfortable reviewing this?

Comment 43

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

cjones, et al: as requested, an overview of what's going on.  This patch exposes and adds support for video recording to the camera control API.  The key DOM-exposed APIs are:
- getPreviewStreamVideoMode()
- startRecording()
- stopRecording()

Once the camera app has obtained a CameraControl object from the mozCamera.getCamera() onsuccess callback, it can either call getPreviewStream() to start the preview in picture-taking mode, or call getPreviewStreamVideoMode() to start the preview in video-taking mode.  (getPreviewStreamVideoMode() can also be called after getPreviewStream(), but _not_ the other way around, due to limitations in the code.  In the case of going from video-taking to picture-taking, the camera must be restarted, which is done by calling getCamera() again.)

Once the onsuccess callback of getPreviewStreamVideoMode() is invoked, the CameraControl is now in recording mode, and the app can call startRecording().  startRecording() creates a suitable file name for the video and configures the GonkRecorder (i.e. mRecorder--see SetupRecording()) and calls start() on it.  This starts the process of consuming raw video frames, compressing them, and writing the resulting stream out to the target file.

(Aside: all of these APIs are defined in nsIDOMCameraManager.idl and initially handled in DOMCameraControl.cpp, which is the DOM-facing cycle-collection participant; but aside from that role, it does very little other than invoke an equivalent handler defined in CameraControlImpl.h/.cpp, which is threadsafe.  The handler in the .cpp file creates an nsRunnable which moves the call off of the main thread and onto the camera thread, where a platform-specific handler in GonkCameraControl.cpp does the heavy lifting.  Interfacing of the preview data to the DOM is handled by DOMCameraPreview.*, which is not part of this patch.)

When you want to stop recording, call stopRecording(), which stops the recorder and triggers the dispatch of the onsuccess callback that was passed to startRecording().  This callback gets the file name into which the video was written.

The code currently only records video in one size (352x288)--see GonkCameraControl.cpp:851--and the DEFAULT_VIDEO_QUALITY macro is used to look up the rest of the video configuration options in /etc/media_profiles.xml.

GonkRecorder.* and AudioParameter.cpp come directly from Android, and are patched at build time by update.sh using the unified diff in update.patch.

If you need more detail, post questions here, so I or Inder can respond to them as appropriate.

Comment 44

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

Comment on attachment 662612 [details] [diff] [review]
Part 1: new media changes with README, update.patch and update.sh

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

::: update.sh
@@ +1,5 @@
> +# Usage: ./update.sh <android_ics_os_src_directory>
> +#
> +# Copies the needed files from the directory containing the original
> +# Android ICS OS source and applies the B2G specific changes for the
> +# camcorder functionality in B2G.

This copies the files into dom/camera/ and then patches them in-place in that folder.  Do we want to be doing this?  (I'm not saying we don't, but I wanted to call attention to it.)

Comment 45

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Mike Habicher [:mikeh] from comment #43)
> cjones, et al: as requested, an overview of what's going on.  This patch
> exposes and adds support for video recording to the camera control API.  The
> key DOM-exposed APIs are:
> - getPreviewStreamVideoMode()
> - startRecording()
> - stopRecording()

These need super-review from a DOM peer.

> 
> Once the camera app has obtained a CameraControl object from the
> mozCamera.getCamera() onsuccess callback, it can either call
> getPreviewStream() to start the preview in picture-taking mode, or call
> getPreviewStreamVideoMode() to start the preview in video-taking mode. 

Why do we need separate interfaces?

> Once the onsuccess callback of getPreviewStreamVideoMode() is invoked, the
> CameraControl is now in recording mode, and the app can call
> startRecording().  startRecording() creates a suitable file name for the
> video and configures the GonkRecorder (i.e. mRecorder--see SetupRecording())
> and calls start() on it.  This starts the process of consuming raw video
> frames, compressing them, and writing the resulting stream out to the target
> file.

How are errors in writing the file handled?

Where are the videos saved?

> When you want to stop recording, call stopRecording(), which stops the
> recorder and triggers the dispatch of the onsuccess callback that was passed
> to startRecording().  This callback gets the file name into which the video
> was written.

Can you describe the filename in more detail?  There's no web API for directly opening files, AFAIK, so I don't know what the recorder could do with this.

Comment 46

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

(In reply to Chris Jones [:cjones] [:warhammer] from comment #45)
> (In reply to Mike Habicher [:mikeh] from comment #43)
> > cjones, et al: as requested, an overview of what's going on.  This patch
> > exposes and adds support for video recording to the camera control API.  The
> > key DOM-exposed APIs are:
> > - getPreviewStreamVideoMode()
> > - startRecording()
> > - stopRecording()
> 
> These need super-review from a DOM peer.

Ack.

> > Once the camera app has obtained a CameraControl object from the
> > mozCamera.getCamera() onsuccess callback, it can either call
> > getPreviewStream() to start the preview in picture-taking mode, or call
> > getPreviewStreamVideoMode() to start the preview in video-taking mode. 
> 
> Why do we need separate interfaces?

Inder, can you answer this question?

> > Once the onsuccess callback of getPreviewStreamVideoMode() is invoked, the
> > CameraControl is now in recording mode, and the app can call
> > startRecording().  startRecording() creates a suitable file name for the
> > video and configures the GonkRecorder (i.e. mRecorder--see SetupRecording())
> > and calls start() on it.  This starts the process of consuming raw video
> > frames, compressing them, and writing the resulting stream out to the target
> > file.
> 
> How are errors in writing the file handled?

Inder, again?

> Where are the videos saved?

The video files are written to /sdcard/Movies, unless that can't be stat()ed, in which case they're written to whatever is returned by |NS_GetSpecialDirectory(NS_OS_TEMP_DIR, getter_AddRefs(f))|.

> > When you want to stop recording, call stopRecording(), which stops the
> > recorder and triggers the dispatch of the onsuccess callback that was passed
> > to startRecording().  This callback gets the file name into which the video
> > was written.
> 
> Can you describe the filename in more detail?  There's no web API for
> directly opening files, AFAIK, so I don't know what the recorder could do
> with this.

The filename is built by calling |strftime(outstr, maxlen, "video_%F__%H-%M-%S", tmp)| and appending an extension (either .3gp or .mp4, depending on the file type returned by |mMediaProfiles->getCamcorderProfileParamByName("file.format", ...)|).  onsuccess gets this as a DOMString, without a fully-qualified path--just the file name.

AFAIK, the recorder doesn't really do anything with it.  The video gallery app is expected to be able find all the video files and show them off.

Comment 47

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

Attachment: Part 2.5: DOM-facing changes (broken out from Part 2)

bz, these DOM-facing changes need a DOM-peer SR.  Can you look it over?

Comment 48

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Mike Habicher [:mikeh] from comment #46)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #45)
> > Where are the videos saved?
> 
> The video files are written to /sdcard/Movies, unless that can't be
> stat()ed, in which case they're written to whatever is returned by
> |NS_GetSpecialDirectory(NS_OS_TEMP_DIR, getter_AddRefs(f))|.
> 

We should fail if /sdcard is unavailable, I think.  We could very quickly and easily fill up /data with recorded video.

> > > When you want to stop recording, call stopRecording(), which stops the
> > > recorder and triggers the dispatch of the onsuccess callback that was passed
> > > to startRecording().  This callback gets the file name into which the video
> > > was written.
> > 
> > Can you describe the filename in more detail?  There's no web API for
> > directly opening files, AFAIK, so I don't know what the recorder could do
> > with this.
> 
> The filename is built by calling |strftime(outstr, maxlen,
> "video_%F__%H-%M-%S", tmp)| and appending an extension (either .3gp or .mp4,
> depending on the file type returned by
> |mMediaProfiles->getCamcorderProfileParamByName("file.format", ...)|). 
> onsuccess gets this as a DOMString, without a fully-qualified path--just the
> file name.
> 

Ah, so the intention is that we could pass the name to mediaStorage.open("...")?  That makes sense.

Comment 49

[Inder]

Attachment: Part 2: review changes (incremental)

This patch incorporates the review comments I received from cdouble and mikeh. It's an incremental patch so needs to be applied on top of WIP patch.
Mike H, can you please incorporate it in the overall WIP patch and also create a new Part 2 patch with this one. After that you can make this obsolete.
Appreciate your help.
-Inder

Comment 50

[Inder]

(In reply to Mike Habicher [:mikeh] from comment #44)
> Comment on attachment 662612 [details] [diff] [review]
> Part 1: new media changes with README, update.patch and update.sh
> 
> Review of attachment 662612 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: update.sh
> @@ +1,5 @@
> > +# Usage: ./update.sh <android_ics_os_src_directory>
> > +#
> > +# Copies the needed files from the directory containing the original
> > +# Android ICS OS source and applies the B2G specific changes for the
> > +# camcorder functionality in B2G.
> 
> This copies the files into dom/camera/ and then patches them in-place in
> that folder.  Do we want to be doing this?  (I'm not saying we don't, but I
> wanted to call attention to it.)

That's intentional. I am just following the same approach as in the media plugin code pointed by cdouble.

Comment 51

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Part 2: camera framework changes, v3(?)

I'm trying to wade my way through this patch but there are a lot of unfamiliar things.  If one of you guys can review this in the meantime, that would be much more efficient.

Comment 52

[Inder]

(In reply to Mike Habicher [:mikeh] from comment #46)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #45)
> > (In reply to Mike Habicher [:mikeh] from comment #43)
> > > cjones, et al: as requested, an overview of what's going on.  This patch
> > > exposes and adds support for video recording to the camera control API.  The
> > > key DOM-exposed APIs are:
> > > - getPreviewStreamVideoMode()
> > > - startRecording()
> > > - stopRecording()
> > 
> > These need super-review from a DOM peer.
> 
> Ack.
> 
> > > Once the camera app has obtained a CameraControl object from the
> > > mozCamera.getCamera() onsuccess callback, it can either call
> > > getPreviewStream() to start the preview in picture-taking mode, or call
> > > getPreviewStreamVideoMode() to start the preview in video-taking mode. 
> > 
> > Why do we need separate interfaces?
> 
> Inder, can you answer this question?

The reason is: getPreviewStream's implementation is different in two modes so had to define separate interfaces. Also, in video mode getPreviewStream gets separate sets of options. Also, the preview dimensions and frame rate could be different so it made sense to define a new interface. In the video mode it gets video options and then does some camcorder related init by calling SetupVideoMode().

> 
> > > Once the onsuccess callback of getPreviewStreamVideoMode() is invoked, the
> > > CameraControl is now in recording mode, and the app can call
> > > startRecording().  startRecording() creates a suitable file name for the
> > > video and configures the GonkRecorder (i.e. mRecorder--see SetupRecording())
> > > and calls start() on it.  This starts the process of consuming raw video
> > > frames, compressing them, and writing the resulting stream out to the target
> > > file.
> > 
> > How are errors in writing the file handled?
> 
> Inder, again?
> 

Once the file is created by GonkCameraControl, as mikeh described, the file descriptor is passed on to GonkRecorder which hands it over to appropriate MediaWriter (e.g., MPEG4Writer) in Android. MediaWriter uses the write() function to write to files and to be honest I don't see much error handling in there. Here is the code for MPEG4Writer: frameworks/base/media/libstagefright/MPEG4Writer.cpp 

> > Where are the videos saved?
> 
> The video files are written to /sdcard/Movies, unless that can't be
> stat()ed, in which case they're written to whatever is returned by
> |NS_GetSpecialDirectory(NS_OS_TEMP_DIR, getter_AddRefs(f))|.
> 

Just a clarification here that it uses the temp dir only if DEFAULT_VIDEO_STORAGE_TO_TEMP_DIR is defined. This was mostly done for testing or in case we want to add this feature. We can easily disable it in the release builds.

> > > When you want to stop recording, call stopRecording(), which stops the
> > > recorder and triggers the dispatch of the onsuccess callback that was passed
> > > to startRecording().  This callback gets the file name into which the video
> > > was written.
> > 
> > Can you describe the filename in more detail?  There's no web API for
> > directly opening files, AFAIK, so I don't know what the recorder could do
> > with this.
> 
> The filename is built by calling |strftime(outstr, maxlen,
> "video_%F__%H-%M-%S", tmp)| and appending an extension (either .3gp or .mp4,
> depending on the file type returned by
> |mMediaProfiles->getCamcorderProfileParamByName("file.format", ...)|). 
> onsuccess gets this as a DOMString, without a fully-qualified path--just the
> file name.
> 
> AFAIK, the recorder doesn't really do anything with it.  The video gallery
> app is expected to be able find all the video files and show them off.

Comment 53

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 665667 [details] [diff] [review]
Part 2.5: DOM-facing changes (broken out from Part 2)

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

I thin it would be much preferable if we passed in the filename to the API rather than have the API pick a location and give it back to us. The problem with having the API pick a location is that it forces the app to move the file after we're done, or otherwise risk that the video won't show up in whatever UI the camera app has. So if we for example crash, or have some bug somewhere causing us to forget to move the file, the file could easily get "leaked".

So change the signature of startRecording to be startRecording(nsIDOMDeviceStorage storageArea, DOMString name, ...);


What happens if someone calls startRecording without first calling getPreviewStreamVideoMode? Probably the right thing to do is to throw. At the very least we should document this in the API.


I also wonder if it would be worth merging getPreviewStreamVideoMode and getPreviewStream such that we just had getPreviewStream({ type: "still"/"video", width: ..., height: ..., rotation: ... }). This part seems less important to me though.


r=me with those things fixed (last one optional).

Comment 54

[cajbir (:cajbir)]

Comment on attachment 662612 [details] [diff] [review]
Part 1: new media changes with README, update.patch and update.sh

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

::: GonkRecorder.cpp
@@ +698,5 @@
> +status_t GonkRecorder::start() {
> +    CHECK(mOutputFd >= 0);
> +
> +    if (mWriter != NULL) {
> +        LOGE("File writer is not avaialble");

Spelling of 'available'.

::: GonkRecorder.h
@@ +47,5 @@
> +    virtual status_t setVideoFrameRate(int frames_per_second);
> +    virtual status_t setOutputFile(const char *path);
> +    virtual status_t setOutputFile(int fd, int64_t offset, int64_t length);
> +    virtual status_t setParameters(const String8& params);
> +    /* CameaHandle */

Spelling. You can actually remove this comment since it doesn't help in any way.

Comment 55

[cajbir (:cajbir)]

Comment on attachment 665704 [details] [diff] [review]
Part 2: camera framework changes, v3(?)

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

::: dom/camera/GonkCameraControl.cpp
@@ +947,5 @@
> +    strncpy(fileName, DEFAULT_VIDEO_FILE_NAME, MAX_VIDEO_FILE_NAME_LEN);
> +  }
> +
> +  // add the extension
> +  strncat(fileName, GetFileExtension(aVideoFileFormat), MAX_VIDEO_FILE_NAME_LEN);

I can't see how you addressed my previous review comment about this being able to write outside the bounds of fileName.

@@ +954,5 @@
> +
> +  struct stat buffer;
> +  int result;
> +  // check if the video storage dir exists
> +  result = stat(VIDEO_STORAGE_DIR, &buffer);

Declare variable and assign in one statement:

int result = ....;

@@ +978,5 @@
> +      DOM_CAMERA_LOGE("Failed to append file name to temp directory\n");
> +      return -1;
> +    }
> +    f->GetNativePath(filePath);
> +    videoFileAbsPath = (char*)filePath.get();

The nsAutoCString 'filePath' goes out of scope after this line leaving a dangling pointer in videoFileAbsPath.

Comment 56

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

(In reply to Jonas Sicking (:sicking) from comment #53)
> Comment on attachment 665667 [details] [diff] [review]
> Part 2.5: DOM-facing changes (broken out from Part 2)
> 
> Review of attachment 665667 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I thin it would be much preferable if we passed in the filename to the API
> rather than have the API pick a location and give it back to us. The problem
> with having the API pick a location is that it forces the app to move the
> file after we're done, or otherwise risk that the video won't show up in
> whatever UI the camera app has. So if we for example crash, or have some bug
> somewhere causing us to forget to move the file, the file could easily get
> "leaked".
> 
> So change the signature of startRecording to be
> startRecording(nsIDOMDeviceStorage storageArea, DOMString name, ...);

With this change we also no longer need to return the filename in the onsuccess handler, and so the onsuccess handler no longer needs to wait until stopRecording() is called in order to get dispatched.  Agreed?

Comment 57

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

What happens if recording errors out somewhere in between startRecording() and stopRecording()?  How would we signal that if we fired onsuccess right after startRecording()?

That change sounds fine to me though.  It sounds like the android libs don't have much error handling anyway.

Comment 58

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

(In reply to Chris Jones [:cjones] [:warhammer] from comment #57)
> What happens if recording errors out somewhere in between startRecording()
> and stopRecording()?  How would we signal that if we fired onsuccess right
> after startRecording()?

I think the onsuccess callback should only indicate that we did, in fact, successfully start recording.  As it is, any errors reported only have to do with starting recording.

A future patch could include a "status update" callback that could be used to also report any errors that happen, or even just to provide a regular recording heartbeat.

> That change sounds fine to me though.  It sounds like the android libs don't
> have much error handling anyway.

And there's that.  :)

Comment 59

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

Attachment: WIP - video recording support (20120928-1333 EDT)

Latest version of the patch, incorporation review feedback.  Will post broken up version shortly.

Comment 60

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

Attachment: camera.js - patch to exercise the recording API

Comment 61

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

Attachment: Part 0: build changes v2

This version includes the update.sh script and the update.patch; a part 1.5 patch will be posted that shows the output of these files.

Comment 62

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

Attachment: Part 1: media changes v2

Doesn't include update.sh or update.patch -- part 1.5 will include the output of those scripts.

Comment 63

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

Attachment: Part 1.5: the output of update.sh and update.patch v1

Comment 64

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

Attachment: Part 2: camera framework changes v4

This sub-patch does not include DOM changes.  Just the camera framework integration.

Comment 65

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

Attachment: Part 2.5: DOM-facing changes v2

Comment 66

[Inder]

(In reply to Jonas Sicking (:sicking) from comment #53)
> Comment on attachment 665667 [details] [diff] [review]
> Part 2.5: DOM-facing changes (broken out from Part 2)
> 
> Review of attachment 665667 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I thin it would be much preferable if we passed in the filename to the API
> rather than have the API pick a location and give it back to us. The problem
> with having the API pick a location is that it forces the app to move the
> file after we're done, or otherwise risk that the video won't show up in
> whatever UI the camera app has. So if we for example crash, or have some bug
> somewhere causing us to forget to move the file, the file could easily get
> "leaked".
> 
> So change the signature of startRecording to be
> startRecording(nsIDOMDeviceStorage storageArea, DOMString name, ...);
> 

There is one problem I see with this. The video configuration values are read from /etc/media_profiles.xml file which along with codec, video/audio bit-rate etc. also tells us which format to save the video in. Based on this requested format we decide the file extension. But, if the API is telling name then I don't see how this will work. Should the API just tell us the base of the file name?

> 
> What happens if someone calls startRecording without first calling
> getPreviewStreamVideoMode? Probably the right thing to do is to throw. At
> the very least we should document this in the API.
> 
> 
> I also wonder if it would be worth merging getPreviewStreamVideoMode and
> getPreviewStream such that we just had getPreviewStream({ type:
> "still"/"video", width: ..., height: ..., rotation: ... }). This part seems
> less important to me though.
> 
> 
> r=me with those things fixed (last one optional).

Comment 67

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

(In reply to Inder from comment #66)
> (In reply to Jonas Sicking (:sicking) from comment #53)
> > Comment on attachment 665667 [details] [diff] [review]
> > Part 2.5: DOM-facing changes (broken out from Part 2)
> > 
> > Review of attachment 665667 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I thin it would be much preferable if we passed in the filename to the API
> > rather than have the API pick a location and give it back to us. The problem
> > with having the API pick a location is that it forces the app to move the
> > file after we're done, or otherwise risk that the video won't show up in
> > whatever UI the camera app has. So if we for example crash, or have some bug
> > somewhere causing us to forget to move the file, the file could easily get
> > "leaked".
> > 
> > So change the signature of startRecording to be
> > startRecording(nsIDOMDeviceStorage storageArea, DOMString name, ...);
> 
> There is one problem I see with this. The video configuration values are
> read from /etc/media_profiles.xml file which along with codec, video/audio
> bit-rate etc. also tells us which format to save the video in. Based on this
> requested format we decide the file extension. But, if the API is telling
> name then I don't see how this will work. Should the API just tell us the
> base of the file name?

For now, the camera app is responsible for providing the .3gp extension, which matches the type exposed by profile 3.  Bug 795379 was created to track a proper way of exposing/doing this.

Comment 68

[Inder]

> For now, the camera app is responsible for providing the .3gp extension,
> which matches the type exposed by profile 3.  Bug 795379 was created to
> track a proper way of exposing/doing this.

Do you want to override the value of mVideoFileFormat with OUTPUT_FORMAT_THREE_GPP to be consistent with the assumption. I have seen media_profiles.xml values change which will break it.

Comment 69

[Inder]

> > +
> > +  // add the extension
> > +  strncat(fileName, GetFileExtension(aVideoFileFormat), MAX_VIDEO_FILE_NAME_LEN);
> 
> I can't see how you addressed my previous review comment about this being
> able to write outside the bounds of fileName.
> 
oops, sorry missed it my patch. Mike H, thanks for addressing it.

Comment 70

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 665982 [details] [diff] [review]
Part 2: camera framework changes v4

(In reply to Chris Double (:doublec) from comment #55)
> Comment on attachment 665704 [details] [diff] [review]
> Part 2: camera framework changes, v3(?)
> 
> Review of attachment 665704 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/camera/GonkCameraControl.cpp
> @@ +947,5 @@
> > +    strncpy(fileName, DEFAULT_VIDEO_FILE_NAME, MAX_VIDEO_FILE_NAME_LEN);
> > +  }
> > +
> > +  // add the extension
> > +  strncat(fileName, GetFileExtension(aVideoFileFormat), MAX_VIDEO_FILE_NAME_LEN);
> 
> I can't see how you addressed my previous review comment about this being
> able to write outside the bounds of fileName.
> 

This code is gone now with the DOM API changes.

> @@ +954,5 @@
> > +
> > +  struct stat buffer;
> > +  int result;
> > +  // check if the video storage dir exists
> > +  result = stat(VIDEO_STORAGE_DIR, &buffer);
> 
> Declare variable and assign in one statement:
> 
> int result = ....;
> 

Same here.

> @@ +978,5 @@
> > +      DOM_CAMERA_LOGE("Failed to append file name to temp directory\n");
> > +      return -1;
> > +    }
> > +    f->GetNativePath(filePath);
> > +    videoFileAbsPath = (char*)filePath.get();
> 
> The nsAutoCString 'filePath' goes out of scope after this line leaving a
> dangling pointer in videoFileAbsPath.

Same here.

Comment 71

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

Try results, good for everything except 'Armv7a ICS opt': https://tbpl.mozilla.org/?tree=Try&rev=d08ea1891cbb

Comment 72

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I.e., the most important one ;).

The problem here is that GonkRecorder.cpp is using two stagefright headers that aren't in our emulator aosp sources

<media/stagefright/ExtendedWriter.h>
<media/stagefright/FMA2DPWriter.h>

without proper runtime detection.  The plan is to just excise the code that depends on these.

Comment 73

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

Attachment: video recording support (20120928-1827 EDT)

try in progress: https://tbpl.mozilla.org/?tree=Try&rev=0487e0627b1e

Comment 74

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Gaia pull request is up at https://github.com/mozilla-b2g/gaia/pull/5501 .  (It doesn't strictly have to wait until the gecko support merges; recording just won't work.)

Comment 75

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

To fix the build with the vanilla toolchain, I stripped out code that looked to be caf extensions.  This stripped out code was added to the patch as dom/camera/update2.patch, and then I added a command to the update.sh script.  This was small and trivial so not requesting re-review.

Comment 76

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/68c4c30ff6f0

Comment 77

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/68c4c30ff6f0

Comment 78

[Doug Turner (:dougt)]

You landed bug 795201 along with this without the cleanup Jonas requested.  Bad form.

Comment 79

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Mike?

Comment 80

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

(In reply to Chris Jones [:cjones] [:warhammer] from comment #79)
> Mike?

Sorry, was so focused on adding app-specified filename support that I missed point 2 on either throwing if the state is bad, or improving the API documentation.  Will file a follow-up patch.

As for Jonas' point 3, I thought we'd agreed to leave the APIs separate for now.

(Personally, I don't like using the preview API to switch camera modes.  There should be a 'setMode()' API that selects video or still, independent of whether or not you want/need a preview stream.)

Comment 81

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

See bug 795201 comment 7. That review comment was never addressed even though you merged in the patch from that bug.

I'm totally fine with leaving the functions separate for now. Point 3 from comment 53 in this bug was explicitly optional.

But yes, please do file a followup on addressing point 2 from comment 53.

Comment 82

[Marco Chen [:mchen]]

I tried to trace the code related to how stagefright library report any error during recording.
I found there is setListener API on GonkRecorder to bypass listener into stagefright library but it seems video recording didn't use this one.

So how does video recording app know the error during recording?
thanks.

Comment 83

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

(In reply to Marco Chen [:mchen] from comment #82)
> I tried to trace the code related to how stagefright library report any
> error during recording.
> I found there is setListener API on GonkRecorder to bypass listener into
> stagefright library but it seems video recording didn't use this one.
> 
> So how does video recording app know the error during recording?
> thanks.

Hi :mchen, I've added bug 801693 to track the asynchronous event plumbing work.  At the very least this will be required to let us know a specified file-size or recording-time limit is reached.