1017471 - Wrong usage of ChangeExplicitBlockerCount in MediaRecorder

Status: RESOLVED FIXED

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

Description

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

Pasue()
http://mxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder.cpp#265
Resume()
http://mxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder.cpp#276

pause should call ChangeExplicitBlockerCount(1);

Comment 1

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

http://dxr.mozilla.org/mozilla-central/source/content/media/MediaStreamGraph.h#566

The blocking counts are unsigned integers. |(uint32_t)-1| is still positive which will result in blocking. However, we should still get the semantics right.

Comment 2

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

It is a good one for newbie to practice how to submit the patches and using the try server.

Comment 3

[Mike Hoye [:mhoye]]

If that's the case, I'm flagging it a good first bug.

Comment 4

[Sam Lin[:neopleo]]

Hi, Is anyone currently working on this bug ?

Comment 5

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

No, as far as I know. You are welcome to take this bug. :)

Comment 6

[Sam Lin[:neopleo]]

Hi, JW, Let me make sure, Is this a gecko bug ?

Comment 7

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

Yes, it's gecko bug.

Comment 8

[Sam Lin[:neopleo]]

Just to make sure a couple of things before taking this bug.

Will I need a device to test this bug ? or Nightly is enough ?

If I need a device to do it, is Flatfish okay ?

Comment 9

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

Nightly will do.

Comment 10

[Sam Lin[:neopleo]]

: ) how to take this bug ?

Comment 11

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

Click the "take" link button after the "Assigned To:" field above.

Comment 12

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

Sorry, I noticed you need the permission to take the bug. I will assign the bug to you. :)

Comment 13

[Sam Lin[:neopleo]]

Hi, I dont see the take link right after the "Assigned To:", would you help me with this?
( maybe due to the insufficient permission to do so ? )

Comment 14

[Sam Lin[:neopleo]]

Thank you.

Comment 15

[Sam Lin[:neopleo]]

BTW, is there anyone who would like to be the mentor to this bug since I have little experience for gecko mdeia framework ?

Comment 16

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

Randy and me will mentor this bug.

Comment 17

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

FYI. https://developer.mozilla.org/en-US/docs/Introduction

Comment 18

[Sam Lin[:neopleo]]

Nice, thanks.

Comment 19

[Sam Lin[:neopleo]]

what's the expected result for this bug? 

Only change 

http://mxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder.cpp#265

from

mTrackUnionStream->ChangeExplicitBlockerCount(-1); 

to 

mTrackUnionStream->ChangeExplicitBlockerCount(1);

???

Comment 20

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

Yes. As well as the one in Resume().
http://mxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder.cpp#282

Comment 21

[Sam Lin[:neopleo]]

http://mxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder.cpp#282

Should this one change 1 to -1 ?

Comment 22

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

Yes. +1 means blocking for Pause() and -1 to restore the blocking change previously made in Pause().

Comment 23

[Sam Lin[:neopleo]]

Attachment: 1017471.patch

This quick patch changes the following variable:

1. In /content/media/MediaRecorder.cpp
   nsresult Pause()
   {
 
     //mTrackUnionStream->ChangeExplicitBlockerCount(-1);
       mTrackUnionStream->ChangeExplicitBlockerCount(1);
   }

2. nsresult Resume()
   {
        //mTrackUnionStream->ChangeExplicitBlockerCount(1);
          mTrackUnionStream->ChangeExplicitBlockerCount(-1);           
   }

Comment 24

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

Comment on attachment 8444907 [details] [diff] [review]
1017471.patch

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

Hi roc,
Can you review this patch?

Comment 25

[Sam Lin[:neopleo]]

Hi JW,

FYI, I have gained the access level 1 permission. Please let me know what is the next step I should do.

Comment 26

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

Hi Sam,
1. Get an r+ from the review peer.
2. Send the patch to Try server for tests to ensure the patch doesn't break anything.
3. Put "checkin-needed" in the Keywords field above and Sheriff will check in the patch for you.

[1] try server: https://wiki.mozilla.org/ReleaseEngineering/TryServer

Comment 27

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

Hi Hi Sam, 
You can refer those documents to learn how to use hg and submit a hg format patch. :)
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Source_Code/Mercurial
https://developer.mozilla.org/en-US/docs/Mercurial

The patch header should like this: 
============
# HG changeset patch
# User Benjamin Chen <[e-mail]>
# Date 1403143894 -28800
#      Thu Jun 19 10:11:34 2014 +0800
# Node ID b01689da977b144fdbfd0cf72a2267c40b0392da
# Parent  9b0718a01b13ef28b27c2bad52bfb3a59dbc94d4
Bug 969372: part2: 1. Let Session hold a reference to MediaRecorder. 2. MediaRecorder implement nsIDocumentActivity. r=roc

diff --git a/content/media/MediaRecorder.cpp b/content/media/MediaRecorder.cpp
--- a/content/media/MediaRecorder.cpp
+++ b/content/media/MediaRecorder.cpp
=your change.=

Comment 28

[Sam Lin[:neopleo]]

Hi JW,

Some questions here:

1. How to get review peer? Did you just request someone to do it?

Hi Hi Randy,

1. only hg format works ? git doesnt ?

Comment 29

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

Yes. You can also click the "details" button to edit a patch to request review from a peer. There is also a "Suggested reviewers" button to display reviewers if you don't know whom to ask.

Comment 30

[Sam Lin[:neopleo]]

Can step 1 and 2 run in the same time? or It needs to wait the approval from reviewer then try server ?

Comment 31

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

Indeed, you should run Try server first to ensure the quality of your patch before asking for review. If the patch is simple and you are confident, you can do both at the same time.

Comment 32

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

Hi Sam,
Git patch can be reviewed, but if Sheriff want to check-in your patch, it should be hg formatted.
Here is the Committing rules and responsibilities you can refer.
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities

Comment 33

[Sam Lin[:neopleo]]

Thank you both. Let me do the review and try server steps at the same time. : )

Comment 34

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

Comment on attachment 8444907 [details] [diff] [review]
1017471.patch

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

Amazing! We need a test for this :-)

In future patches please include at least 8 lines of context. Thanks!

Comment 35

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

Hi Sam,

Here is a reference for you to configure your Mecurial.
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 36

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #34)
> Amazing! We need a test for this :-)

What kind of test do we need?

Comment 37

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

Something calls Pause() that can actually check if the stream was paused?

Comment 38

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

http://hg.mozilla.org/mozilla-central/file/f78e532e8a10/content/media/MediaStreamGraph.cpp#l778

Both ChangeExplicitBlockerCount(1) and ChangeExplicitBlockerCount(-1) will cause |explicitBlock| to be true for (uint32_t)-1 is still positive. It is fine if there is only one changing the blocking count of a stream in a reverse way (1 <=> -1). Things will be different if there are multiple clients changing the blocking count for clientA.Pause(+1) && MediaRecorder.Pause(-1) will result in non-blocking instead of blocking with count = 2.

I was thinking about passing an enum (BLOCKED or UNBLOCKED) to ChangeExplicitBlockerCount() since we should never pass something other than 1 or -1.

Comment 39

[Sam Lin[:neopleo]]

Hi,

Please let me know the next step I should proceed to or just wait here until any conclusion to be made.

Comment 40

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

Hi Sam,
Please follow https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F and https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities to generate a well-formed patch. Then we can ask sheriff to check in the patch.

Comment 41

[Sam Lin[:neopleo]]

Hi JW,

I will attach a hg formatted patch soon.

Comment 42

[Sam Lin[:neopleo]]

Attachment: 1017471_hg.patch

Dear JW and Randy,

I've attached the patch in hg format, it looks very similar to what Randy posted at @27, please help check this. Thanks!

Comment 43

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

Hi Sam,
It is nearly done. Please also include the commit message in your patch. For such a minor fix (commit message or tailing spaces), you can just set the flag to r+ without review again.

ref: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Commit_message_restrictions

Comment 44

[Sam Lin[:neopleo]]

Attachment: bug_1017471_hg.patch

Hi JW,

Thanks! I've added one line for commit message. 
Please review it again and let me know if this one works.

Comment 45

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

Almost there. Please also inlcude the reviewer in the commit message. :)
ex: Bug 101741 - minor fix for the wrong usage of ChangeExplicitBlockerCount in MediaRecorder. r=roc.

Comment 46

[Sam Lin[:neopleo]]

Attachment: bug_1017471_hg.patch

Hi JW,

I've modified the patch.

Please find the attachment and let me know what to do next.

Thank you. : )

Comment 47

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

Hi Sam,

Now you can click the "Details" button -> "edit details" to check the "obsolete" check box to discard the patches that are obsolete. Then put the keyword "checkin-needed" in the keywords field to notice Sheriff to check in the patch for you.

Comment 48

[Sam Lin[:neopleo]]

Hi JW,

Thanks. I've done all things you mentioned @ comment 47.

So now we just wait the Sheriff to check in this bug and close it?

Comment 49

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

You put "checkin-needed" in the wrong field.
Yes, Sheriff will take care of the rest.

Comment 50

[Sam Lin[:neopleo]]

Oh! sorry. Thank you JW and Randy.

Comment 51

[Carsten Book [:Tomcat]]

Sam, can you also provide a try server link for this patches ? Thanks!

Comment 52

[Sam Lin[:neopleo]]

Hi Carsten,

Yes, I can. I will provide you a try server link later.

Comment 53

[Sam Lin[:neopleo]]

Hi Carsten,

Just to make sure that the try server link you refer to is the builds and logs link, right ?

Comment 54

[Sam Lin[:neopleo]]

Hi Carsten,

With your comment related to the try server link,

1. Results are in the following link:
   https://tbpl.mozilla.org/?tree=Try&rev=79bc7c426e30

2. Builds and logs are available in the following link:
   https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-79bc7c426e30

So far I've only had these links. Please let me know if any further information you may need.

Comment 55

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a32bd8a8b6f

Comment 56

[Sam Lin[:neopleo]]

I just had a brief conversation with the sheriff and this patch has been landed. 
Thanks to mentors and Carsten.

Comment 57

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

Sheriff will switch bug status when the patch passes verification. You just need to put "check-in-needed" and let Sheriff take care of the rest.

Comment 58

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5a32bd8a8b6f