Closed Bug 930444 Opened 7 years ago Closed 6 years ago

[MediaEncoder] Add Profile Label in Encoding path

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: u459114, Assigned: alwu, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 5 obsolete files)

Add profile label to address encoding pipeline performance issue in pseudostack.

https://developer.mozilla.org/en-US/docs/Performance/Profiling_with_the_Built-in_Profiler
Assignee: nobody → rlin
Component: Video/Audio → Video/Audio: Recording
Free this if anyone wants.
Assignee: rlin → nobody
Assignee: nobody → alwu
Hi Ehsan, 
Sorry to bother you. 
We tried to add this label on "Media Encoder" thread but we still can't find this tag shown on cleopatra. 
Did I miss something? Should I add this thread name on somewhere?
Flags: needinfo?(ehsan.akhgari)
(In reply to Randy Lin [:rlin] from comment #2)
> Hi Ehsan, 
> Sorry to bother you. 
> We tried to add this label on "Media Encoder" thread but we still can't find
> this tag shown on cleopatra. 
> Did I miss something? Should I add this thread name on somewhere?

Can I see a patch?  :)
Flags: needinfo?(ehsan.akhgari) → needinfo?(rlin)
Did you call profiler_register_thread?
Attached patch test patch 1 (obsolete) — Splinter Review
I just have a test on this but got a crash on 
F/MOZ_Assert( 6757): Assertion failure: false, at ../../../../../../media/ssd/gecko_c2/tools/profiler/platform-linux.cc:465

Did I miss something?
Flags: needinfo?(rlin)
Attached patch test patch 1.1 (obsolete) — Splinter Review
Fix incorrect profiler_unregister_thread location.
But I still get the same crash problem...
(In reply to Randy Lin [:rlin] from comment #5)
> Created attachment 8496723 [details] [diff] [review]
> test patch 1
> 
> I just have a test on this but got a crash on 
> F/MOZ_Assert( 6757): Assertion failure: false, at
> ../../../../../../media/ssd/gecko_c2/tools/profiler/platform-linux.cc:465
> 
> Did I miss something?

Did you read the comment right above that MOZ_ASSERT(false)?  <http://mxr.mozilla.org/mozilla-central/source/tools/profiler/platform-linux.cc#463>  It clearly explains what's happening here.  You're registering the same thread multiple times.
Attached patch Patch v1 (obsolete) — Splinter Review
Hi, Ehasn
I add some profiling label in the media encoding pipeline, could you help me review my patch?
Thanks :)
Attachment #8496723 - Attachment is obsolete: true
Attachment #8496737 - Attachment is obsolete: true
Attachment #8499472 - Flags: feedback?(ehsan.akhgari)
It's my profiling result.
Comment on attachment 8499472 [details] [diff] [review]
Patch v1

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

Nit: please create the patches with 8 lines of context for review.  Thanks!

On the patch itself, the PROFILER_LABEL additions themselves look fine, but I don't know this code very well, so I don't feel comfortable reviewing this.  Things to note during the review is the threading model (see the comment below), and also, whether any of these functions can be hot (since pushing and poping these labels can be costly, and therefore adding too many labels may skew the profile.)  Perhaps try cpearce?

::: content/media/MediaRecorder.cpp
@@ +439,4 @@
>      MOZ_ASSERT(NS_GetCurrentThread() == mReadThread);
>      LOG(PR_LOG_DEBUG, ("Session.Extract %p", this));
>  
> +    if (!mIsRegisterProfiler) {

I am not sure about the threading model of the code involved here, but please make sure that this doesn't race, i.e., there can only ever be one thread entering this code for the first time.  Otherwise you'll have a race condition on two threads trying to call profiler_register_thread.

::: content/media/encoder/fmp4_muxer/ISOMediaWriter.h
@@ +8,4 @@
>  
>  #include "ContainerWriter.h"
>  #include "nsIRunnable.h"
> +#include "GeckoProfiler.h"

Nit: please move this to the .cpp file, so that all consumers of this header don't pull in GeckoProfiler.h needlessly.
Attachment #8499472 - Flags: feedback?(ehsan.akhgari) → feedback?(cpearce)
Comment on attachment 8499472 [details] [diff] [review]
Patch v1

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

I don't know this code very well, so I'd prefer you to find someone else to look at it. Probably roc is the best bet. He reviewed most of MediaRecorder I think.
Attachment #8499472 - Flags: feedback?(cpearce)
Attached patch Patch v2 (obsolete) — Splinter Review
Hi, Robert.
Sorry to bother you, could you help me review my patch?
Thanks!
Attachment #8500215 - Flags: feedback?(roc)
Comment on attachment 8500215 [details] [diff] [review]
Patch v2

Switch to review flag. :)
Attachment #8500215 - Flags: feedback?(roc) → review?(roc)
Keywords: checkin-needed
Add the profiler_unregister_thread() and remove some labels in MediaRecorder.
Attachment #8499472 - Attachment is obsolete: true
Attachment #8500215 - Attachment is obsolete: true
Attachment #8508407 - Attachment is obsolete: true
Hi Alastor, 

could you provide a try run to make sure everything works and nothing breaks.

Thanks!
Keywords: checkin-needed
(In reply to Carsten Book [:Tomcat] from comment #16)
> Hi Alastor, 
> 
> could you provide a try run to make sure everything works and nothing breaks.
> 
> Thanks!

Here is the try-server record, thanks!
https://tbpl.mozilla.org/?tree=Try&rev=ce204a31ed28
https://hg.mozilla.org/mozilla-central/rev/3708fa125c56
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.