Closed Bug 950429 Opened 10 years ago Closed 10 years ago

Media Recording - allow NS_DispatchToCurrentThread in Media Encoder thread

Categories

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

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: rlin, Assigned: rlin)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 5 obsolete files)

If we use NS_DispatchToCurrentThread in Media Encoder thread, we found this task would run before thread exit.
Assignee: nobody → rlin
Attached patch patch v1 (obsolete) — Splinter Review
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.
Attachment #8351146 - Flags: review?(roc)
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
Attachment #8351146 - Flags: review?(roc) → review-
Attached patch patch v2 (obsolete) — Splinter Review
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...
Attachment #8351146 - Attachment is obsolete: true
Attachment #8351342 - Flags: review?(roc)
(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? :-)
Attached patch patch v2.1 (obsolete) — Splinter Review
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.
Attachment #8351342 - Attachment is obsolete: true
Attachment #8351342 - Flags: review?(roc)
Attachment #8351445 - Flags: review?(roc)
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
Attachment #8351445 - Flags: review?(roc) → review-
(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.
Attached patch patch v3 (obsolete) — Splinter Review
Hold Sesion by nsTArray. also fix nits.
Attachment #8351445 - Attachment is obsolete: true
Attachment #8354967 - Flags: review?(roc)
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?
OK, I will do it.
Attached patch patch v4 (obsolete) — Splinter Review
Remove the code refactoring part.
Attachment #8354967 - Attachment is obsolete: true
Attachment #8354967 - Flags: review?(roc)
Attachment #8355784 - Flags: review?(roc)
Attachment #8355784 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/49191bc5ad7a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Whiteboard: [qa-]
Component: Video/Audio → Video/Audio: Recording
No longer blocks: MediaRecording
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: