833521 - recording events in Content Process are not delivered to chrome process

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Sotaro Ikeda [:sotaro]]

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]]

I recommend adding an observer to ContentChild and forwarding the notification across IPDL, then redispatching it.

Comment 2

[Sotaro Ikeda [:sotaro]]

(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]]

Attachment: dom camera diagram

I recognized the problem during I was creating the diagram.

Comment 4

[Mike Habicher [:mikeh] (high bugzilla latency)]

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]]

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]]

(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]]

Attachment: patch: forward recording events

forward "recording-device-events" events frome content process to chrome process

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: patch rev2: forward recording events

remove unintended change to ContainerChild's super class.

Comment 9

[Mike Habicher [:mikeh] (high bugzilla latency)]

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)]

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]]

Attachment: 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)]

(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]]

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)]

If this gets reviewed in time (as per akeybl's emails) I'll take care of landing it.

Comment 15

[Doug Turner (:dougt)]

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]]

(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]]

Attachment: 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]]

Comment on attachment 705844 [details] [diff] [review]
patch rev4: forward recording events

dougt, can you review the patch again?

Comment 19

[Doug Turner (:dougt)]

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]]

Attachment: 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)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/58209cc0ae14

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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]]

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)]

/mood is: humbled.
try-server push: https://tbpl.mozilla.org/?tree=Try&rev=cd423973d9ab

Comment 25

[Mike Habicher [:mikeh] (high bugzilla latency)]

(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)]

Attachment: patch rev6: forward recording events (try version)

Carrying r=dougt forward

Comment 27

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: patch rev7: forward recording events (try version, win32 fix)

Carrying r=dougt forward

Comment 28

[Mike Habicher [:mikeh] (high bugzilla latency)]

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=036db0daa3fe

Comment 29

[Mike Habicher [:mikeh] (high bugzilla latency)]

I hate you, GetCurrentTime.

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=f5e0d794663f

Comment 30

[Mike Habicher [:mikeh] (high bugzilla latency)]

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]]

(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)]

(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)]

try: https://tbpl.mozilla.org/?tree=Try&rev=8acde6c7a93a

Comment 34

[Mike Habicher [:mikeh] (high bugzilla latency)]

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)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/58c0eb12bf23

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ef08d673ef20

Comment 37

[Lukas Blakk [:lsblakk] use ?needinfo]

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.