recording events in Content Process are not delivered to chrome process

RESOLVED FIXED in Firefox 21

Status

()

Core
Audio/Video
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sotaro, Assigned: sotaro)

Tracking

unspecified
mozilla21
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:tef+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed, b2g18-v1.0.0 fixed)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

5 years ago
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

5 years ago
I recommend adding an observer to ContentChild and forwarding the notification across IPDL, then redispatching it.
(Assignee)

Updated

5 years ago
Blocks: 774581
(Assignee)

Comment 2

5 years ago
(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.
(Assignee)

Updated

5 years ago
Assignee: nobody → sotaro.ikeda.g
(Assignee)

Comment 3

5 years ago
Created attachment 705071 [details]
dom camera diagram

I recognized the problem during I was creating the diagram.
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?
(Assignee)

Comment 5

5 years ago
It appear when Camera App is loaded in chrome process. And bug 828600 re-appear when the problem fixed. I confirmed locally.
(Assignee)

Updated

5 years ago
Blocks: 828600
(Assignee)

Comment 6

5 years ago
(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.
(Assignee)

Comment 7

5 years ago
Created attachment 705096 [details] [diff] [review]
patch: forward recording events

forward "recording-device-events" events frome content process to chrome process
(Assignee)

Comment 8

5 years ago
Created attachment 705099 [details] [diff] [review]
patch rev2: forward recording events

remove unintended change to ContainerChild's super class.
Attachment #705096 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #705099 - Attachment is patch: true
(Assignee)

Updated

5 years ago
Depends on: 833571
(Assignee)

Updated

5 years ago
blocking-b2g: --- → tef?
(Assignee)

Updated

5 years ago
Attachment #705099 - Flags: review?(mhabicher)
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?
Attachment #705099 - Flags: review?(mhabicher) → review?(doug.turner)
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.
(Assignee)

Comment 11

5 years ago
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?
(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.
(Assignee)

Comment 13

5 years ago
Comment on attachment 705528 [details] [diff] [review]
patch rev3: forward recording events

dougt, can you review the patch?
Attachment #705528 - Flags: review?(doug.turner)
(Assignee)

Updated

5 years ago
Attachment #705099 - Attachment is obsolete: true
Attachment #705099 - Flags: review?(doug.turner)
blocking-b2g: tef? → tef+
If this gets reviewed in time (as per akeybl's emails) I'll take care of landing it.
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?
(Assignee)

Comment 16

5 years ago
(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"
(Assignee)

Comment 17

5 years ago
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"
Attachment #705528 - Attachment is obsolete: true
Attachment #705528 - Flags: review?(doug.turner)
(Assignee)

Comment 18

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

dougt, can you review the patch again?
Attachment #705844 - Flags: review?(doug.turner)
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.
Attachment #705844 - Flags: review?(doug.turner) → review+
(Assignee)

Comment 20

5 years ago
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()->
>
Attachment #705844 - Attachment is obsolete: true
Attachment #705972 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/58209cc0ae14
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.
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
/mood is: humbled.
try-server push: https://tbpl.mozilla.org/?tree=Try&rev=cd423973d9ab
(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.
Created attachment 706025 [details] [diff] [review]
patch rev6: forward recording events (try version)

Carrying r=dougt forward
Attachment #705972 - Attachment is obsolete: true
Attachment #706025 - Flags: review+
Created attachment 706065 [details] [diff] [review]
patch rev7: forward recording events (try version, win32 fix)

Carrying r=dougt forward
Attachment #706025 - Attachment is obsolete: true
Attachment #706065 - Flags: review+
try-server push: https://tbpl.mozilla.org/?tree=Try&rev=036db0daa3fe
I hate you, GetCurrentTime.

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=f5e0d794663f
Maybe this is the magic frob that unhoses the Windows build?

try: https://tbpl.mozilla.org/?tree=Try&rev=c87917726b6c
(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)
(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.
try: https://tbpl.mozilla.org/?tree=Try&rev=8acde6c7a93a
The try push is looking good.  m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef08d673ef20
https://hg.mozilla.org/releases/mozilla-b2g18/rev/58c0eb12bf23
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
status-b2g18: --- → fixed
status-firefox19: --- → wontfix
status-firefox20: --- → wontfix
status-firefox21: --- → fixed
Target Milestone: --- → mozilla21
https://hg.mozilla.org/mozilla-central/rev/ef08d673ef20
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.
status-b2g18-v1.0.0: --- → fixed
You need to log in before you can comment on or make changes to this bug.