951008 - Media Recorder - fire onstart event when encoder start to generate encoded data

Status: RESOLVED FIXED

(Core :: Audio/Video: Recording, defect)

Description

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

On 
https://dvcs.w3.org/hg/dap/raw-file/tip/media-stream-capture/MediaRecorder.html#widl-MediaRecorder-onstart
Start method: 
Once data becomes available raise a start event and start gathering the data into a Blob (see [FILE-API]).

Comment 1

[Jason Smith [:jsmith]]

This isn't related to any MozTrap test case that exists right now - this is a feature doesn't exist & does not have test cases mapped to it.

Comment 2

[Roland Kunkel [:RolandK]]

Fails test case: https://moztrap.mozilla.org/manage/case/11593/

Comment 3

[Roland Kunkel [:RolandK]]

Console enabled must be checked under Developer settings to see the onstart event. Removing whiteboard tag as it no longer fails the test case listed in comment #2.

Comment 4

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Free it if someone want it.

Comment 5

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v1

First patch for adding the onstart event.
Also contain the test case for this event.

Comment 6

[John Lin [:jhlin][:jolin]]

Comment on attachment 8446354 [details] [diff] [review]
patch v1

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

::: content/media/MediaRecorder.cpp
@@ +347,1 @@
>        mEncodedBufferCache->AppendBuffer(encodedBuf[i]);

Nitty: Don't have to do this when hasData is false.

::: content/media/test/test_mediarecorder_record_getdata_afterstart.html
@@ +47,5 @@
> +  mMediaRecorder.onstop = function() {
> +    info('onstop fired successfully');
> +  };
> +
> +  mMediaRecorder.onstop = function() {

Why assign mMediaRecorder.onstop twice?

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8446354 [details] [diff] [review]
patch v1

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

::: content/media/test/test_mediarecorder_record_getdata_afterstart.html
@@ +24,5 @@
> +  var dest = context.createMediaStreamDestination();
> +  source.connect(dest);
> +  var elem = document.createElement('audio');
> +  elem.mozSrcObject = dest.stream;
> +  mMediaStream = dest.stream;

Why not just capture a stream from an audio element instead of going a long way through WebAudio?

@@ +55,5 @@
> +  };
> +
> +  mMediaRecorder.ondataavailable = function (e) {
> +    if (mMediaRecorder.state == 'recording') {
> +      ok(hasonstart, "should has onstart event first"); 

space

@@ +56,5 @@
> +
> +  mMediaRecorder.ondataavailable = function (e) {
> +    if (mMediaRecorder.state == 'recording') {
> +      ok(hasonstart, "should has onstart event first"); 
> +      is('audio/ogg', mMediaRecorder.mimeType, "check the record mimetype return " + mMediaRecorder.mimeType);

MIME type depends on available codec on the system which depends on pref settings. This check doesn't guarantee to be valid.

Comment 8

[Alfredo Yang (:alfredo)]

Comment on attachment 8446354 [details] [diff] [review]
patch v1

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

::: content/media/MediaRecorder.cpp
@@ +120,5 @@
> +  // Fire start event and set mimeType, run in main thread task.
> +  class DispatchStartEventRunnable : public nsRunnable
> +  {
> +  public:
> +	DispatchStartEventRunnable(Session* aSession, const nsAString & aEventName)

nit

@@ +133,5 @@
> +
> +      nsRefPtr<MediaRecorder> recorder = mSession->mRecorder;
> +      if (!recorder) {
> +        return NS_OK;
> +      }

How about this?

NS_ENSURE_TRUE(mSession->mRecorder, NS_OK);
nsRefPtr<MediaRecorder> recorder = mSession->mRecorder;

@@ +134,5 @@
> +      nsRefPtr<MediaRecorder> recorder = mSession->mRecorder;
> +      if (!recorder) {
> +        return NS_OK;
> +      }
> +      recorder->SetMimeType(mSession->mMimeType);

It looks like recorder->SetMimeType/GetMimeType are both accessed in main thread. Do we need Muxtex in http://dxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder.h#111?

@@ +347,5 @@
>        mEncodedBufferCache->AppendBuffer(encodedBuf[i]);
> +      // Fire the start event when encoded data is available.
> +      if (!mCanRetrieveData && hasData) {
> +        NS_DispatchToMainThread(
> +          new DispatchStartEventRunnable(this,NS_LITERAL_STRING("start")));

nit

@@ +508,5 @@
>    // by calling requestData API.
>    const int32_t mTimeSlice;
>    // Indicate this session's stop has been called.
>    bool mStopIssued;
> +  // Indicate session has encoded data. can be changed in read thread.

Missing subject in second sentence?
So, what is "read thread"?

::: content/media/test/test_mediarecorder_record_getdata_afterstart.html
@@ +5,5 @@
> +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
> +  <script type="text/javascript" src="manifest.js"></script>
> +</head>
> +<body>

bug id and url?

Comment 9

[Benjamin Chen [:bechen]]

Comment on attachment 8446354 [details] [diff] [review]
patch v1

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

::: content/media/MediaRecorder.cpp
@@ +509,5 @@
>    const int32_t mTimeSlice;
>    // Indicate this session's stop has been called.
>    bool mStopIssued;
> +  // Indicate session has encoded data. can be changed in read thread.
> +  bool mCanRetrieveData;

Since the flag used only once for fire onstart event, I think the naming of the variable should be changed.... 
|mCanRetrieveData| tells me that the session has encoded data.

Comment 10

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v2

Correct those nits, thanks for your feedback.

About the mimeType in Session problem, I had a check and those operation is in the main thread. So I don't add mutex for that one. And the GetMimeType/SetMimeType funcitons have mutex to protect it.

About the mCanRetrieveData naming, because the client can get the encoded data after onstart event, so it imply we already have encoded data for application.

Comment 11

[Alfredo Yang (:alfredo)]

(In reply to Randy Lin [:rlin] from comment #10)
> Created attachment 8446434 [details] [diff] [review]
> patch v2
> 
> Correct those nits, thanks for your feedback.
> 
> About the mimeType in Session problem, I had a check and those operation is
> in the main thread. So I don't add mutex for that one. And the
> GetMimeType/SetMimeType funcitons have mutex to protect it.

Hmmm...
I mean it DOES NOT need a muxtex for GetMimeType/SetMimeType functions.

Comment 12

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8446434 [details] [diff] [review]
patch v2

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

::: content/media/test/test_mediarecorder_record_getdata_afterstart.html
@@ +59,5 @@
> +    }
> +  };
> +
> +  // Start recording once canplaythrough fires
> +  element.oncanplaythrough = function() {

You should listen to 'loadedmetadata' instead of 'canplaythrough' for 'canplaythrough' will not necessarily fire depending on download speed.

Comment 13

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v2.1

Hi Roc,
Could you review this?

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8447000 [details] [diff] [review]
patch v2.1

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

::: content/media/test/test_mediarecorder_record_getdata_afterstart.html
@@ +37,5 @@
> +  mMediaRecorder.onstart = function() {
> +    info('onstart fired successfully');
> +    hasonstart = true;
> +    // On audio only case, we produce audio/ogg as mimeType.
> +    ok('audio/ogg', mMediaRecorder.mimeType, "check the record mimetype return " + mMediaRecorder.mimeType);

This should be "is" not "ok"!

@@ +60,5 @@
> +  };
> +
> +  // Start recording once canplaythrough fires
> +  element.onloadedmetadata = function() {
> +    element.oncanplaythrough = null;

Fix comment and clearing this event handler

Comment 15

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: check-in patch

Carry reviewer as roc.
Fix 2 nits.
Try result
  https://tbpl.mozilla.org/?tree=Try&rev=a6a0adc6706d

Comment 16

[Carsten Book [:Tomcat]]

checkin failed:
remote: WebIDL file dom/webidl/MediaRecorder.webidl altered in changeset 541378c6c4ad without DOM peer review
remote: Changes to WebIDL files in this repo require review from a DOM peer in the form of r=...

so i guess this needs dom peer review ?

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8447834 [details] [diff] [review]
check-in patch

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

::: content/media/MediaRecorder.cpp
@@ +341,5 @@
>      mEncoder->GetEncodedData(&encodedBuf, mMimeType);
>  
>      // Append pulled data into cache buffer.
>      for (uint32_t i = 0; i < encodedBuf.Length(); i++) {
> +      if (encodedBuf[i].Length() > 0) {

IsEmpty() exists

Comment 18

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v2.2

Hi Kyle, 
Could you help to review the API part?

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8449124 [details] [diff] [review]
patch v2.2

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

You mean the WebIDL change?  r=me.

Comment 20

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: check-in patch

Thanks khuey for reviewing the webidl part.
Carry sr = khuey.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3110f58de690

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3110f58de690