Closed
Bug 833521
Opened 12 years ago
Closed 12 years ago
recording events in Content Process are not delivered to chrome process
Categories
(Core :: Audio/Video, defect)
Tracking
()
People
(Reporter: sotaro, Assigned: sotaro)
References
Details
Attachments
(2 files, 6 obsolete files)
113.38 KB,
application/pdf
|
Details | |
9.26 KB,
patch
|
mikeh
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
I recommend adding an observer to ContentChild and forwarding the notification across IPDL, then redispatching it.
Assignee | ||
Comment 2•12 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•12 years ago
|
Assignee: nobody → sotaro.ikeda.g
Assignee | ||
Comment 3•12 years ago
|
||
I recognized the problem during I was creating the diagram.
Comment 4•12 years ago
|
||
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•12 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 | ||
Comment 6•12 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•12 years ago
|
||
forward "recording-device-events" events frome content process to chrome process
Assignee | ||
Comment 8•12 years ago
|
||
remove unintended change to ContainerChild's super class.
Attachment #705096 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #705099 -
Attachment is patch: true
Assignee | ||
Updated•12 years ago
|
blocking-b2g: --- → tef?
Assignee | ||
Updated•12 years ago
|
Attachment #705099 -
Flags: review?(mhabicher)
Comment 9•12 years ago
|
||
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)
Comment 10•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
(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•12 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•12 years ago
|
Attachment #705099 -
Attachment is obsolete: true
Attachment #705099 -
Flags: review?(doug.turner)
Updated•12 years ago
|
blocking-b2g: tef? → tef+
Comment 14•12 years ago
|
||
If this gets reviewed in time (as per akeybl's emails) I'll take care of landing it.
Comment 15•12 years ago
|
||
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•12 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•12 years ago
|
||
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•12 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 19•12 years ago
|
||
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•12 years ago
|
||
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+
Comment 21•12 years ago
|
||
Comment 22•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
/mood is: humbled.
try-server push: https://tbpl.mozilla.org/?tree=Try&rev=cd423973d9ab
Comment 25•12 years ago
|
||
(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•12 years ago
|
||
Carrying r=dougt forward
Attachment #705972 -
Attachment is obsolete: true
Attachment #706025 -
Flags: review+
Comment 27•12 years ago
|
||
Carrying r=dougt forward
Attachment #706025 -
Attachment is obsolete: true
Attachment #706065 -
Flags: review+
Comment 28•12 years ago
|
||
try-server push: https://tbpl.mozilla.org/?tree=Try&rev=036db0daa3fe
Comment 29•12 years ago
|
||
I hate you, GetCurrentTime.
try-server push: https://tbpl.mozilla.org/?tree=Try&rev=f5e0d794663f
Comment 30•12 years ago
|
||
Maybe this is the magic frob that unhoses the Windows build?
try: https://tbpl.mozilla.org/?tree=Try&rev=c87917726b6c
Comment 31•12 years ago
|
||
(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•12 years ago
|
||
(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•12 years ago
|
||
Comment 34•12 years ago
|
||
The try push is looking good. m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef08d673ef20
Comment 35•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
status-b2g18:
--- → fixed
status-firefox19:
--- → wontfix
status-firefox20:
--- → wontfix
status-firefox21:
--- → fixed
Target Milestone: --- → mozilla21
Comment 36•12 years ago
|
||
Comment 37•12 years ago
|
||
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.
Description
•