Closed Bug 833521 Opened 8 years ago Closed 8 years ago

recording events in Content Process are not delivered to chrome process

Categories

(Core :: Audio/Video, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21
blocking-b2g tef+
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(2 files, 6 obsolete files)

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.
I recommend adding an observer to ContentChild and forwarding the notification across IPDL, then redispatching it.
Blocks: 774581
(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: nobody → sotaro.ikeda.g
Attached file 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?
It appear when Camera App is loaded in chrome process. And bug 828600 re-appear when the problem fixed. I confirmed locally.
Blocks: 828600
(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.
Attached patch patch: forward recording events (obsolete) — Splinter Review
forward "recording-device-events" events frome content process to chrome process
remove unintended change to ContainerChild's super class.
Attachment #705096 - Attachment is obsolete: true
Attachment #705099 - Attachment is patch: true
Depends on: 833571
blocking-b2g: --- → tef?
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.
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.
Comment on attachment 705528 [details] [diff] [review]
patch rev3: forward recording events

dougt, can you review the patch?
Attachment #705528 - Flags: review?(doug.turner)
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?
(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"
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)
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+
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+
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
(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.
Carrying r=dougt forward
Attachment #705972 - Attachment is obsolete: true
Attachment #706025 - Flags: review+
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.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/58c0eb12bf23
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
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.
You need to log in before you can comment on or make changes to this bug.