950429 - Media Recording - allow NS_DispatchToCurrentThread in Media Encoder thread

Status: RESOLVED FIXED

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

Description

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

If we use NS_DispatchToCurrentThread in Media Encoder thread, we found this task would run before thread exit.

Comment 1

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

Attachment: patch v1

Remove the while loop in Session and use dispatch runnable to media_encoder thread. 
Also change the fire simpleEvent/reqeustData model to reduce the coherence between Session and MediaRecorder.

Comment 2

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

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

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

I don't understand from the bug or the comments in this patch why we're making these changes.

::: content/media/MediaRecorder.cpp
@@ +71,3 @@
>    {
>    public:
> +	NotifyMediaRecorderEventRunnable(Session* aSession, uint8_t aEveitID, nsresult aRv)

Fix typo "aEveitID". These values need documentation! Maybe you should use an enum?

@@ +160,5 @@
> +    SESSION_HASDATA,
> +    SESSION_STOP,
> +    SESSION_ERROR,
> +    SESSION_UNKNOWN,
> +  };

Yes! Name this enum and use it for the event ID variables.

@@ +302,5 @@
>  
>      // In case source media stream does not notify track end, recieve
>      // shutdown notification and stop Read Thread.
>      nsContentUtils::RegisterShutdownObserver(this);
> +    mlastBlobTimeStamp= TimeStamp::Now();

Space before =

@@ +325,5 @@
>      MOZ_ASSERT(NS_IsMainThread());
>  
>      if (strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID) == 0) {
>        // Force stop Session to terminate Read Thread.
> +      if (mEncoder && !mEncoder->IsShutdown()){

Space before {

@@ +353,5 @@
>    // The interval of passing encoded data from EncodedBufferCache to onDataAvailable
>    // handler. "mTimeSlice < 0" means Session object does not push encoded data to
>    // onDataAvailable, instead, it passive wait the client side pull encoded data
>    // by calling requestData API.
> +  TimeStamp mlastBlobTimeStamp;

mLast

Comment 3

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

Attachment: patch v2

Fix nits.
I found the Session object do many tasks that should be done by MediaRecorder object. So I try to refine code on this one. 

The void RecorderEvent(const uint8_t aSessionEventType still use uint8_t because the header can't refer that enum definition...

Comment 4

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

(In reply to Randy Lin [:rlin] from comment #3)
> Created attachment 8351342 [details] [diff] [review]
> patch v2
> 
> Fix nits.
> I found the Session object do many tasks that should be done by
> MediaRecorder object. So I try to refine code on this one.

Can you be more specific?

> The void RecorderEvent(const uint8_t aSessionEventType still use uint8_t
> because the header can't refer that enum definition...

So, move it? :-)

Comment 5

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

Attachment: patch v2.1

In session class, we have used the recorder object and invoke some function call like
NotifyError
CreateAndDispatchBlobEvent
Stop
DispatchSimpleEvent and etc...
I think those tasks should relay back to recorder object and make session clear and easy to maintain.

Comment 6

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

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

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

::: content/media/MediaRecorder.cpp
@@ +344,5 @@
>    nsRefPtr<MediaEncoder> mEncoder;
>    // A buffer to cache encoded meda data.
>    nsAutoPtr<EncodedBufferCache> mEncodedBufferCache;
> +  // Record the last fired dataavailable event timestamp.
> +  TimeStamp mLastBlobTimeStamp;

Why have you moved this out to become a field instead of a local variable?

@@ +527,5 @@
>  
>    BlobEventInit init;
>    init.mBubbles = false;
>    init.mCancelable = false;
> +  init.mData = mSession->GetEncodedData();

I'm not convinced this is a good idea.

How can we be sure that we don't lose data when one session is replaced by another? E.g. if someone calls recorder.stop(); recorder.start();, if the old session requests dispatching of a blob event, won't we look in the new session for data and the data from the old session will be forgotten?

::: content/media/MediaRecorder.h
@@ +103,5 @@
>    void NotifyError(nsresult aRv);
>    // Check if the recorder's principal is the subsume of mediaStream
>    bool CheckPrincipal();
> +  // Receive event from Session object.
> +  void RecorderEvent(const SessionEventType aSessionEventType, const nsresult aRv);

Don't need "const" here

Comment 7

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> Comment on attachment 8351445 [details] [diff] [review]
> patch v2.1
> 
> Review of attachment 8351445 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/MediaRecorder.cpp
> @@ +344,5 @@
> >    nsRefPtr<MediaEncoder> mEncoder;
> >    // A buffer to cache encoded meda data.
> >    nsAutoPtr<EncodedBufferCache> mEncodedBufferCache;
> > +  // Record the last fired dataavailable event timestamp.
> > +  TimeStamp mLastBlobTimeStamp;
> 
> Why have you moved this out to become a field instead of a local variable?
I move the Extract method to ExtractRunnable class and use DispatchToCurrentThread to replace the while loop to retrieve data. So for timeslice requirement, I need a last blob time for reference.

> @@ +527,5 @@
> >  
> >    BlobEventInit init;
> >    init.mBubbles = false;
> >    init.mCancelable = false;
> > +  init.mData = mSession->GetEncodedData();
> 
> I'm not convinced this is a good idea.
> 
> How can we be sure that we don't lose data when one session is replaced by
> another? E.g. if someone calls recorder.stop(); recorder.start();, if the
> old session requests dispatching of a blob event, won't we look in the new
> session for data and the data from the old session will be forgotten?
ah.....maybe media recorder should have a array to manage the session objects. 

> ::: content/media/MediaRecorder.h
> @@ +103,5 @@
> >    void NotifyError(nsresult aRv);
> >    // Check if the recorder's principal is the subsume of mediaStream
> >    bool CheckPrincipal();
> > +  // Receive event from Session object.
> > +  void RecorderEvent(const SessionEventType aSessionEventType, const nsresult aRv);
> 
> Don't need "const" here
ok.

Comment 8

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

Attachment: patch v3

Hold Sesion by nsTArray. also fix nits.

Comment 9

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

I don't think this patch is making things cleaner and easier to maintain. This array-of-sessions approach seems to make the code worse.

Can we have one patch to fix the problem that this bug is about, and do any restructuring of sessions in a separate patch?

Comment 10

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

OK, I will do it.

Comment 11

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

Attachment: patch v4

Remove the code refactoring part.

Comment 12

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

Attachment: check-in patch

check-in patch, carry reviewer, try result
https://tbpl.mozilla.org/?tree=Try&rev=0ee5f6ec9450

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/49191bc5ad7a

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/49191bc5ad7a