Last Comment Bug 833521 - recording events in Content Process are not delivered to chrome process
: recording events in Content Process are not delivered to chrome process
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla21
Assigned To: Sotaro Ikeda [:sotaro]
:
: Maire Reavy [:mreavy]
Mentors:
Depends on: 833571
Blocks: 774581 828600
  Show dependency treegraph
 
Reported: 2013-01-22 12:31 PST by Sotaro Ikeda [:sotaro]
Modified: 2013-01-30 15:56 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
tef+
wontfix
wontfix
fixed
fixed
fixed


Attachments
dom camera diagram (113.38 KB, application/pdf)
2013-01-22 12:44 PST, Sotaro Ikeda [:sotaro]
no flags Details
patch: forward recording events (5.60 KB, patch)
2013-01-22 13:25 PST, Sotaro Ikeda [:sotaro]
no flags Details | Diff | Splinter Review
patch rev2: forward recording events (5.56 KB, patch)
2013-01-22 13:33 PST, Sotaro Ikeda [:sotaro]
no flags Details | Diff | Splinter Review
patch rev3: forward recording events (7.97 KB, patch)
2013-01-23 13:26 PST, Sotaro Ikeda [:sotaro]
no flags Details | Diff | Splinter Review
patch rev4: forward recording events (7.89 KB, patch)
2013-01-24 07:03 PST, Sotaro Ikeda [:sotaro]
doug.turner: review+
Details | Diff | Splinter Review
patch rev5: forward recording events (7.96 KB, patch)
2013-01-24 10:59 PST, Sotaro Ikeda [:sotaro]
sotaro.ikeda.g: review+
Details | Diff | Splinter Review
patch rev6: forward recording events (try version) (8.42 KB, patch)
2013-01-24 12:57 PST, Mike Habicher [:mikeh] (high bugzilla latency)
bugzilla: review+
Details | Diff | Splinter Review
patch rev7: forward recording events (try version, win32 fix) (9.26 KB, patch)
2013-01-24 14:09 PST, Mike Habicher [:mikeh] (high bugzilla latency)
bugzilla: review+
Details | Diff | Splinter Review

Description Sotaro Ikeda [:sotaro] 2013-01-22 12:31:36 PST
In B2G, recording status event "recording-device-events" is emitted by camera dom in ContentProcess by using ObserverService. The event is needed to be delivered to chrome process. But it is not.

The event is used to control recording indicator in status bar in System App.

The ObserverService is process local service. Someone needs to deliver the event from content process to chrome process.
Comment 1 Josh Matthews [:jdm] (on vacation until Dec 5) 2013-01-22 12:33:52 PST
I recommend adding an observer to ContentChild and forwarding the notification across IPDL, then redispatching it.
Comment 2 Sotaro Ikeda [:sotaro] 2013-01-22 12:37:25 PST
(In reply to Josh Matthews [:jdm] from comment #1)
> I recommend adding an observer to ContentChild and forwarding the
> notification across IPDL, then redispatching it.

Yeah, I am preparing the path.
Comment 3 Sotaro Ikeda [:sotaro] 2013-01-22 12:44:19 PST
Created attachment 705071 [details]
dom camera diagram

I recognized the problem during I was creating the diagram.
Comment 4 Mike Habicher [:mikeh] (high bugzilla latency) 2013-01-22 12:47:02 PST
Thanks for working on this.  One question (perhaps for paul): I'm sure at some point in the past I saw recording notification icons in the status bar.  How did those get there?
Comment 5 Sotaro Ikeda [:sotaro] 2013-01-22 12:50:33 PST
It appear when Camera App is loaded in chrome process. And bug 828600 re-appear when the problem fixed. I confirmed locally.
Comment 6 Sotaro Ikeda [:sotaro] 2013-01-22 12:57:20 PST
(In reply to Sotaro Ikeda [:sotaro] from comment #5)
> It appear when Camera App is loaded in chrome process. And bug 828600
> re-appear when the problem fixed. I confirmed locally.

Until recently, Camera app from lock screen is loaded in chrome process.
Comment 7 Sotaro Ikeda [:sotaro] 2013-01-22 13:25:32 PST
Created attachment 705096 [details] [diff] [review]
patch: forward recording events

forward "recording-device-events" events frome content process to chrome process
Comment 8 Sotaro Ikeda [:sotaro] 2013-01-22 13:33:19 PST
Created attachment 705099 [details] [diff] [review]
patch rev2: forward recording events

remove unintended change to ContainerChild's super class.
Comment 9 Mike Habicher [:mikeh] (high bugzilla latency) 2013-01-23 08:39:33 PST
Comment on attachment 705099 [details] [diff] [review]
patch rev2: forward recording events

This looks good to me, but I'm by no means an expert on the Observer service and IPDL.  dougt, can you take a look?
Comment 10 Doug Turner (:dougt) 2013-01-23 09:23:40 PST
Mike, do you only notify from C++?

If so, it would be simpler if you just do something like:


http://mxr.mozilla.org/mozilla-central/source/dom/camera/DOMCameraControl.cpp#259


259   obs->NotifyObservers(nullptr,
260                        "recording-device-events",
261                        NS_LITERAL_STRING("starting").get());

// Some sort of comment that describes that you need to remote this event
if (XRE_GetProcessType() != GeckoProcessType_Default) {

  unused << ContentChild::GetSingleton()->SendRecordingDeviceEvents(NS_LITERAL_STRING("starting"));
}

Doing it this way allows you to not have to have the ContentChild not implement nsIObserver.
Comment 11 Sotaro Ikeda [:sotaro] 2013-01-23 13:26:34 PST
Created attachment 705528 [details] [diff] [review]
patch rev3: forward recording events

Apply dougt's comment.
mikeh, the event is right now notified only from C++. And it is correct in the near future, I think.
How do you think about it?
Comment 12 Mike Habicher [:mikeh] (high bugzilla latency) 2013-01-23 13:32:31 PST
(In reply to Sotaro Ikeda [:sotaro] from comment #11)
> 
> Apply dougt's comment.

Can you get dougt to review it?

> mikeh, the event is right now notified only from C++. And it is correct in
> the near future, I think.  How do you think about it?

That's likely the case, yes.  If not, we can address it later.
Comment 13 Sotaro Ikeda [:sotaro] 2013-01-23 14:27:15 PST
Comment on attachment 705528 [details] [diff] [review]
patch rev3: forward recording events

dougt, can you review the patch?
Comment 14 Mike Habicher [:mikeh] (high bugzilla latency) 2013-01-23 21:46:20 PST
If this gets reviewed in time (as per akeybl's emails) I'll take care of landing it.
Comment 15 Doug Turner (:dougt) 2013-01-23 22:02:59 PST
Comment on attachment 705528 [details] [diff] [review]
patch rev3: forward recording events

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

::: dom/ipc/ContentParent.cpp
@@ +1214,5 @@
>  #endif
>  }
>  
> +bool
> +ContentParent::RecvRecordingDeviceEvents(const nsString& aVolumeName)

aVolumeName?  Isn't this the type of recording event?

::: dom/ipc/ContentParent.h
@@ +346,5 @@
>      virtual bool RecvAudioChannelChangedNotification();
>  
>      virtual bool RecvBroadcastVolume(const nsString& aVolumeName);
>  
> +    virtual bool RecvRecordingDeviceEvents(const nsString& aVolumeName);

Same.

::: dom/ipc/PContent.ipdl
@@ +432,5 @@
>                                 nsCString aReason);
>      // get nsIVolumeService to broadcast volume information
>      async BroadcastVolume(nsString volumeName);
>  
> +    async RecordingDeviceEvents(nsString data);

nsString aType?

::: dom/media/MediaManager.h
@@ +21,5 @@
>  namespace mozilla {
>  
>  class GetUserMediaNotificationEvent: public nsRunnable
>  {
> +  typedef mozilla::dom::ContentChild ContentChild;

shouldn't this be

using mozilla::dom?
Comment 16 Sotaro Ikeda [:sotaro] 2013-01-24 07:00:25 PST
(In reply to Doug Turner (:dougt) from comment #15)
> Comment on attachment 705528 [details] [diff] [review]
> patch rev3: forward recording events
> 
> Review of attachment 705528 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/ipc/ContentParent.cpp
> @@ +1214,5 @@
> >  #endif
> >  }
> >  
> > +bool
> > +ContentParent::RecvRecordingDeviceEvents(const nsString& aVolumeName)
> 
> aVolumeName?  Isn't this the type of recording event?

change to "aRecordingStatus"

> 
> ::: dom/ipc/ContentParent.h
> @@ +346,5 @@
> >      virtual bool RecvAudioChannelChangedNotification();
> >  
> >      virtual bool RecvBroadcastVolume(const nsString& aVolumeName);
> >  
> > +    virtual bool RecvRecordingDeviceEvents(const nsString& aVolumeName);
> 
> Same.

change to "aRecordingStatus".

> 
> ::: dom/ipc/PContent.ipdl
> @@ +432,5 @@
> >                                 nsCString aReason);
> >      // get nsIVolumeService to broadcast volume information
> >      async BroadcastVolume(nsString volumeName);
> >  
> > +    async RecordingDeviceEvents(nsString data);
> 
> nsString aType?

I am goting to change it to "recordingStatus" to align with above "aRecordingStatus".

> 
> ::: dom/media/MediaManager.h
> @@ +21,5 @@
> >  namespace mozilla {
> >  
> >  class GetUserMediaNotificationEvent: public nsRunnable
> >  {
> > +  typedef mozilla::dom::ContentChild ContentChild;
> 
> shouldn't this be
> 
> using mozilla::dom?

Change to "using mozilla::dom::ContentChild"
Comment 17 Sotaro Ikeda [:sotaro] 2013-01-24 07:03:10 PST
Created attachment 705844 [details] [diff] [review]
patch rev4: forward recording events

apply comments.
 - change arguments names.
 - replace "typedef mozilla::dom::ContentChild ContentChild" by "using mozilla::dom::ContentChild"
Comment 18 Sotaro Ikeda [:sotaro] 2013-01-24 07:06:05 PST
Comment on attachment 705844 [details] [diff] [review]
patch rev4: forward recording events

dougt, can you review the patch again?
Comment 19 Doug Turner (:dougt) 2013-01-24 10:33:21 PST
Comment on attachment 705844 [details] [diff] [review]
patch rev4: forward recording events

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

::: dom/media/MediaManager.h
@@ +19,3 @@
>  #include "mozilla/Attributes.h"
>  
> +using mozilla::dom::ContentChild;

since this is in a header file, you may just want to directly use:

  mozilla::dom::ContentChild::GetSingleton()->

Sorry for the confusion in the last review.
Comment 20 Sotaro Ikeda [:sotaro] 2013-01-24 10:59:22 PST
Created attachment 705972 [details] [diff] [review]
patch rev5: forward recording events

carry doug.turner: review+.

address following comment. 
> 
> since this is in a header file, you may just want to directly use:
> 
>   mozilla::dom::ContentChild::GetSingleton()->
>
Comment 21 Mike Habicher [:mikeh] (high bugzilla latency) 2013-01-24 11:53:17 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/58209cc0ae14
Comment 22 Ryan VanderMeulen [:RyanVM] 2013-01-24 12:26:17 PST
Backed out for build bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3630b8ef3c0c

https://tbpl.mozilla.org/php/getParsedLog.php?id=19099489&tree=Mozilla-Inbound

/builds/slave/m-in-osx64/build/dom/media/MediaManager.cpp:43:9: error: 'LOG' macro redefined [-Werror]
#define LOG(msg)
        ^
/builds/slave/m-in-osx64/build/ipc/chromium/src/base/logging.h:95:9: note: previous definition is here
#define LOG(info) mozilla::LogWrapper(mozilla::LOG_ ## info, __FILE__, __LINE__)
        ^
PYTHONDONTWRITEBYTECODE=1 /builds/slave/m-in-osx64/build/obj-firefox/i386/_virtualenv/bin/python /builds/slave/m-in-osx64/build/config/pythonpath.py \
	  -I/builds/slave/m-in-osx64/build/other-licenses/ply -I/builds/slave/m-in-osx64/build/dom/bindings/parser \
	  /builds/slave/m-in-osx64/build/dom/bindings/BindingGen.py cpp \
	  /builds/slave/m-in-osx64/build/dom/bindings/Bindings.conf AudioParamBinding \
	  AudioParam.webidl
1 error generated.
Comment 23 Daniel Holbert [:dholbert] 2013-01-24 12:32:31 PST
So what's happening there is:
 - MediaManager.cpp has a pre-existing #define for LOG().
 - ipc/chromium/.../logging.h also has a #define for LOG().
 - One of the #includes added to MediaManager.h presumably gets it to (indirectly) #include that ipc/chromium logging.h file.
 - So: now MediaManager has two LOG() definitions.

Two ways to resolve this:
*hacky but simple: add an "#undef LOG" before the "#define LOG" lines in MediaManager.cpp
*slightly cleaner: replace LOG with somethinge else (e.g. "MLOG") in MediaManager.cpp
Comment 24 Mike Habicher [:mikeh] (high bugzilla latency) 2013-01-24 12:45:08 PST
/mood is: humbled.
try-server push: https://tbpl.mozilla.org/?tree=Try&rev=cd423973d9ab
Comment 25 Mike Habicher [:mikeh] (high bugzilla latency) 2013-01-24 12:50:25 PST
(In reply to Daniel Holbert [:dholbert] from comment #23)
> 
> Two ways to resolve this:
> *hacky but simple: add an "#undef LOG" before the "#define LOG" lines in
> MediaManager.cpp
> *slightly cleaner: replace LOG with something else (e.g. "MLOG") in
> MediaManager.cpp

I prefer the latter option, but I went with the former since it's more future-proof.
Comment 26 Mike Habicher [:mikeh] (high bugzilla latency) 2013-01-24 12:57:48 PST
Created attachment 706025 [details] [diff] [review]
patch rev6: forward recording events (try version)

Carrying r=dougt forward
Comment 27 Mike Habicher [:mikeh] (high bugzilla latency) 2013-01-24 14:09:38 PST
Created attachment 706065 [details] [diff] [review]
patch rev7: forward recording events (try version, win32 fix)

Carrying r=dougt forward
Comment 28 Mike Habicher [:mikeh] (high bugzilla latency) 2013-01-24 14:10:09 PST
try-server push: https://tbpl.mozilla.org/?tree=Try&rev=036db0daa3fe
Comment 29 Mike Habicher [:mikeh] (high bugzilla latency) 2013-01-24 15:10:20 PST
I hate you, GetCurrentTime.

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=f5e0d794663f
Comment 30 Mike Habicher [:mikeh] (high bugzilla latency) 2013-01-24 16:34:59 PST
Maybe this is the magic frob that unhoses the Windows build?

try: https://tbpl.mozilla.org/?tree=Try&rev=c87917726b6c
Comment 31 Daniel Holbert [:dholbert] 2013-01-24 16:43:27 PST
(In reply to Mike Habicher [:mikeh] from comment #30)
> Maybe this is the magic frob that unhoses the Windows build?

From that push:
https://hg.mozilla.org/try/rev/50f7be4a5261#l2.1
> 2.2 +++ b/dom/camera/nsIDOMCameraManager.idl
[...]
> +// undef the GetCurrentTime macro defined in WinBase.h from the MS Platform SDK
> +%{C++
> +#ifdef GetCurrentTime
> +#undef GetCurrentTime
> +#endif
> +%}

Shouldn't that go in nsIDOMMediaStream.idl, not nsIDOMCameraManager.idl? (since nsIDOMMediaStream is the thing that generates the "currentTime" getter that causes problems, in the try run from comment 29)
Comment 32 Mike Habicher [:mikeh] (high bugzilla latency) 2013-01-24 17:04:19 PST
(In reply to Daniel Holbert [:dholbert] from comment #31)
>
> Shouldn't that go in nsIDOMMediaStream.idl, not nsIDOMCameraManager.idl?
> (since nsIDOMMediaStream is the thing that generates the "currentTime"
> getter that causes problems, in the try run from comment 29)

I'll try that next.
Comment 33 Mike Habicher [:mikeh] (high bugzilla latency) 2013-01-24 17:07:36 PST
try: https://tbpl.mozilla.org/?tree=Try&rev=8acde6c7a93a
Comment 34 Mike Habicher [:mikeh] (high bugzilla latency) 2013-01-24 18:46:21 PST
The try push is looking good.  m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef08d673ef20
Comment 35 Mike Habicher [:mikeh] (high bugzilla latency) 2013-01-24 20:15:51 PST
https://hg.mozilla.org/releases/mozilla-b2g18/rev/58c0eb12bf23
Comment 36 Ryan VanderMeulen [:RyanVM] 2013-01-26 16:55:34 PST
https://hg.mozilla.org/mozilla-central/rev/ef08d673ef20
Comment 37 Lukas Blakk [:lsblakk] use ?needinfo 2013-01-30 15:56:01 PST
Landed on mozilla-b2g18/gaia master prior to the 1/25 branching to mozilla-b2g18_v1_0_0/v1.0.0, updating status-b2g-v1.0.0 to fixed.

Note You need to log in before you can comment on or make changes to this bug.