Add PreferredAudioOutputDeviceManager implementation. [chromium/src : main]
Sunggook Chue (Gerrit)
Sunggook Chue added 1 comment![]( "Open in Gerrit")
Related details
- Olga Sharonova
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olga Sharonova (Gerrit)
Olga Sharonova added 8 comments![]( "Open in Gerrit")
std::string preferred_sin_id;preferred_sink_id
AudioOutputDeviceSwitcher* broker,It's not a broker, it's AudioOutputDeviceSwitcher - let's not make leaky abstractions by disclosing implementation details via names.
// streaming is active. The cached sink id will be used to set the
// preferred sink id for the audio service when the audio service is started.I don't quite understand this part.
// forwards the request to the corresponding active audio controler ifThere is no concept of audio controller at this layer, it's deep in the audio service.
// forwards the request to the corresponding active audio controler if"controler" is a possible misspelling of "controller".
Please fix.
// It caches the preferred sink id with each top RenderFrameHost ojbect and"ojbect" is a possible misspelling of "object".
Please fix.
weak_ptr_factory_.GetWeakPtr(), broker,Which thread PreferredAudioOutputDeviceManager is destroyed on?
weak_ptr can't be used on IO thread, if the PreferredAudioOutputDeviceManager is destroyed on UI thread (see base/memory/weak_ptr.h)
SetPreferredSinkId(string sink_id) => (media.mojom.OutputDeviceStatus status);Why this is a part of this CL?
Related details
- Olga Sharonova
- Sunggook Chue
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sunggook Chue (Gerrit)
Sunggook Chue added 9 comments![]( "Open in Gerrit")
addressed and replied comments, thanks.
std::string preferred_sin_id;Sunggook Chuepreferred_sink_id
Done
It's not a broker, it's AudioOutputDeviceSwitcher - let's not make leaky abstractions by disclosing implementation details via names.
Done
// streaming is active. The cached sink id will be used to set the
// preferred sink id for the audio service when the audio service is started.I don't quite understand this part.
Done
// forwards the request to the corresponding active audio controler ifThere is no concept of audio controller at this layer, it's deep in the audio service.
Done
// forwards the request to the corresponding active audio controler if"controler" is a possible misspelling of "controller".
Please fix.
Done
// It caches the preferred sink id with each top RenderFrameHost ojbect and"ojbect" is a possible misspelling of "object".
Please fix.
Done
weak_ptr_factory_.GetWeakPtr(), broker,Which thread PreferredAudioOutputDeviceManager is destroyed on?
weak_ptr can't be used on IO thread, if the PreferredAudioOutputDeviceManager is destroyed on UI thread (see base/memory/weak_ptr.h)
PreferredAudioOutputDeviceManager is created and destroyed on IO thread like VideoCaptureManager.
I don't find weak_ptr (WeakPtr) can't be on IO thread although it enforce use of same thread sequence.
PreferredAudioOutputDeviceManager lifetime belongs MediaStreamManager that also has VideoCaptureManager, which has VideoCaptureController.
VideoCaptureController is also created and destroyed like PreferredAudioOutputDeviceManager, and it is WeakPtrFactory.
Can you point out any guideline, i might missed that?
SetPreferredSinkId(string sink_id) => (media.mojom.OutputDeviceStatus status);Why this is a part of this CL?
PreferredAudioOutputDeviceManager::SetPreferredSinkId function needs 'SetPreferredSinkIdCallback', which come from this.
Related details
- Olga Sharonova
- Olga Sharonova
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olga Sharonova (Gerrit)
Olga Sharonova added 3 comments![]( "Open in Gerrit")
weak_ptr_factory_.GetWeakPtr(), broker,Sunggook ChueWhich thread PreferredAudioOutputDeviceManager is destroyed on?
weak_ptr can't be used on IO thread, if the PreferredAudioOutputDeviceManager is destroyed on UI thread (see base/memory/weak_ptr.h)
PreferredAudioOutputDeviceManager is created and destroyed on IO thread like VideoCaptureManager.
I don't find weak_ptr (WeakPtr) can't be on IO thread although it enforce use of same thread sequence.
PreferredAudioOutputDeviceManager lifetime belongs MediaStreamManager that also has VideoCaptureManager, which has VideoCaptureController.
VideoCaptureController is also created and destroyed like PreferredAudioOutputDeviceManager, and it is WeakPtrFactory.
Can you point out any guideline, i might missed that?
PreferredAudioOutputDeviceManager is created and destroyed on IO thread like VideoCaptureManager.
VideoCaptureManager is refcounted, so it's unknown on which thread it is destroyed. Also, it does not seem to use weak pointers?
VideoCaptureController is the one who uses weak pointers, and not necessarily correctly, since it also seems to be refcounted. I don't know what guarantees the correctness there (probably nothing), we may need ask the authors of the code.
Also, media stream manager does not seem to be destroyed on IO thread, rather IO thread is destroyed before: https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/media/media_stream_manager.cc;l=1643;drc=7b232da0f22e8cdf555d43c52b6491baeb87f729;bpv=1;bpt=1
Guido knows this part better.
SetPreferredSinkId(string sink_id) => (media.mojom.OutputDeviceStatus status);Sunggook ChueWhy this is a part of this CL?
PreferredAudioOutputDeviceManager::SetPreferredSinkId function needs 'SetPreferredSinkIdCallback', which come from this.
It does not come from this one, it comes from AudioOutputDeviceSwitcher.
Related details
- Olga Sharonova
- Sunggook Chue
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sunggook Chue (Gerrit)
Sunggook Chue added 4 comments![]( "Open in Gerrit")
addressed and replied comments, thanks.
weak_ptr_factory_.GetWeakPtr(), broker,Sunggook ChueWhich thread PreferredAudioOutputDeviceManager is destroyed on?
weak_ptr can't be used on IO thread, if the PreferredAudioOutputDeviceManager is destroyed on UI thread (see base/memory/weak_ptr.h)
Olga SharonovaPreferredAudioOutputDeviceManager is created and destroyed on IO thread like VideoCaptureManager.
I don't find weak_ptr (WeakPtr) can't be on IO thread although it enforce use of same thread sequence.
PreferredAudioOutputDeviceManager lifetime belongs MediaStreamManager that also has VideoCaptureManager, which has VideoCaptureController.
VideoCaptureController is also created and destroyed like PreferredAudioOutputDeviceManager, and it is WeakPtrFactory.
Can you point out any guideline, i might missed that?
PreferredAudioOutputDeviceManager is created and destroyed on IO thread like VideoCaptureManager.
VideoCaptureManager is refcounted, so it's unknown on which thread it is destroyed. Also, it does not seem to use weak pointers?
VideoCaptureController is the one who uses weak pointers, and not necessarily correctly, since it also seems to be refcounted. I don't know what guarantees the correctness there (probably nothing), we may need ask the authors of the code.Also, media stream manager does not seem to be destroyed on IO thread, rather IO thread is destroyed before: https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/media/media_stream_manager.cc;l=1643;drc=7b232da0f22e8cdf555d43c52b6491baeb87f729;bpv=1;bpt=1
Guido knows this part better.
VideoCaptureManager (PreferredAudioOutputDeviceManager) will be destroyed in the IO thread during DestructionObserver::WillDestroyCurrentMessageLoop and MediaStreamManager is destroyed on non IO thread after this.
SetPreferredSinkId(string sink_id) => (media.mojom.OutputDeviceStatus status);Sunggook ChueWhy this is a part of this CL?
Olga SharonovaPreferredAudioOutputDeviceManager::SetPreferredSinkId function needs 'SetPreferredSinkIdCallback', which come from this.
It does not come from this one, it comes from AudioOutputDeviceSwitcher.
if delete it from the media_devices, the compilation error.
../..\content/browser/renderer_host/media/media_devices_dispatcher_host.h(80,27): error: unknown type name 'SetPreferredSinkIdCallback'
80 | SetPreferredSinkIdCallback callback) override;
Related details
- Gabriel Brito
- Guido Urdaneta
- Olga Sharonova
- Olga Sharonova
- Steve Becker
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guido Urdaneta (Gerrit)
Guido Urdaneta added 1 comment![]( "Open in Gerrit")
weak_ptr_factory_.GetWeakPtr(), broker,Sunggook ChueWhich thread PreferredAudioOutputDeviceManager is destroyed on?
weak_ptr can't be used on IO thread, if the PreferredAudioOutputDeviceManager is destroyed on UI thread (see base/memory/weak_ptr.h)
Olga SharonovaPreferredAudioOutputDeviceManager is created and destroyed on IO thread like VideoCaptureManager.
I don't find weak_ptr (WeakPtr) can't be on IO thread although it enforce use of same thread sequence.
PreferredAudioOutputDeviceManager lifetime belongs MediaStreamManager that also has VideoCaptureManager, which has VideoCaptureController.
VideoCaptureController is also created and destroyed like PreferredAudioOutputDeviceManager, and it is WeakPtrFactory.
Can you point out any guideline, i might missed that?
Sunggook ChuePreferredAudioOutputDeviceManager is created and destroyed on IO thread like VideoCaptureManager.
VideoCaptureManager is refcounted, so it's unknown on which thread it is destroyed. Also, it does not seem to use weak pointers?
VideoCaptureController is the one who uses weak pointers, and not necessarily correctly, since it also seems to be refcounted. I don't know what guarantees the correctness there (probably nothing), we may need ask the authors of the code.Also, media stream manager does not seem to be destroyed on IO thread, rather IO thread is destroyed before: https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/media/media_stream_manager.cc;l=1643;drc=7b232da0f22e8cdf555d43c52b6491baeb87f729;bpv=1;bpt=1
Guido knows this part better.
VideoCaptureManager (PreferredAudioOutputDeviceManager) will be destroyed in the IO thread during DestructionObserver::WillDestroyCurrentMessageLoop and MediaStreamManager is destroyed on non IO thread after this.
Like Olga says, MediaStreamManager is **not** destroyed on the IO thread. It outlives the IO thread.
Related details
- Gabriel Brito
- Olga Sharonova
- Olga Sharonova
- Steve Becker
- Sunggook Chue
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guido Urdaneta (Gerrit)
Guido Urdaneta added 1 comment![]( "Open in Gerrit")
NOTIMPLEMENTED();Follow Olga's indications on how to split the CLs, and make sure this appears in the same CL where the method is added to the mojom file and that it has a real implementation.
Olga Sharonova (Gerrit)
Olga Sharonova added 7 comments![]( "Open in Gerrit")
PreferredAudioOutputDeviceManger introduces 2 interaction surfaces: 1) Interface for brokers to register/undergister; 2) SetPreferredSinkId interface.
We need to see the code
1) handling PreferredAudioOutputDeviceManger lifetime.
2) introducing an interaction between a broker and PreferredAudioOutputDeviceManger.
3) introducing SetPreferredSinkId usage.
It's impossible to reason about correctness here until we see the interactions with the class.
This should likely be 3 separate CLs, but I leave it up to you to arrange it so it makes sense.
Also a diagram of relationships would be very helpful.
void AddBrokerAndGetOverrideDeviceId(
AudioOutputDeviceSwitcher* device_switcher,
base::OnceCallback<void(const std::string&)> callback);
void GetOverrideDeviceId(
const int process_id,
const int render_frame_id,
base::OnceCallback<void(const std::string&)> callback);
void RemoveBrokerEntry(AudioOutputDeviceSwitcher* device_switcher);
void RemoveRenderFrameHostEntry(RenderFrameHost* render_frame_host);
I'd like to see a CL which clearly introduces usage of these interfaces. Only looking at it we can judge this one. Currently the usage is introduces in the last CL of the chain along with many other things, so we can't review effectively.
void AddBrokerAndGetOverrideDeviceId(Re: naming: we agreed to used Switcher; this class is unaware of brokers. Please fix everywhere.
// streaming is active. The cached sink id will be used instead of the activeWhat does "streaming is active" mean?
// forwards the request to the corresponding active audio service ifWhat does it mean?
weak_ptr_factory_.GetWeakPtr(), broker,Sunggook ChueWhich thread PreferredAudioOutputDeviceManager is destroyed on?
weak_ptr can't be used on IO thread, if the PreferredAudioOutputDeviceManager is destroyed on UI thread (see base/memory/weak_ptr.h)
Olga SharonovaPreferredAudioOutputDeviceManager is created and destroyed on IO thread like VideoCaptureManager.
I don't find weak_ptr (WeakPtr) can't be on IO thread although it enforce use of same thread sequence.
PreferredAudioOutputDeviceManager lifetime belongs MediaStreamManager that also has VideoCaptureManager, which has VideoCaptureController.
VideoCaptureController is also created and destroyed like PreferredAudioOutputDeviceManager, and it is WeakPtrFactory.
Can you point out any guideline, i might missed that?
Sunggook ChuePreferredAudioOutputDeviceManager is created and destroyed on IO thread like VideoCaptureManager.
VideoCaptureManager is refcounted, so it's unknown on which thread it is destroyed. Also, it does not seem to use weak pointers?
VideoCaptureController is the one who uses weak pointers, and not necessarily correctly, since it also seems to be refcounted. I don't know what guarantees the correctness there (probably nothing), we may need ask the authors of the code.Also, media stream manager does not seem to be destroyed on IO thread, rather IO thread is destroyed before: https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/media/media_stream_manager.cc;l=1643;drc=7b232da0f22e8cdf555d43c52b6491baeb87f729;bpv=1;bpt=1
Guido knows this part better.
Guido UrdanetaVideoCaptureManager (PreferredAudioOutputDeviceManager) will be destroyed in the IO thread during DestructionObserver::WillDestroyCurrentMessageLoop and MediaStreamManager is destroyed on non IO thread after this.
Like Olga says, MediaStreamManager is **not** destroyed on the IO thread. It outlives the IO thread.
VideoCaptureManager (PreferredAudioOutputDeviceManager) will be destroyed in the IO thread during DestructionObserver::WillDestroyCurrentMessageLoop
Please point to the place in the code you refer to.
Also please document the reasoning bout the lifetime in the code. Why it's safe to use weak pointers, why the object will be destroyed on IO thread, etc.
Also please document the object threading: created on thread x, requests are forwarded to thread y, destroyed on thread z.
SetPreferredSinkId(string sink_id) => (media.mojom.OutputDeviceStatus status);Sunggook ChueWhy this is a part of this CL?
Olga SharonovaPreferredAudioOutputDeviceManager::SetPreferredSinkId function needs 'SetPreferredSinkIdCallback', which come from this.
Sunggook ChueIt does not come from this one, it comes from AudioOutputDeviceSwitcher.
if delete it from the media_devices, the compilation error.
../..\content/browser/renderer_host/media/media_devices_dispatcher_host.h(80,27): error: unknown type name 'SetPreferredSinkIdCallback'
80 | SetPreferredSinkIdCallback callback) override;
Because MediaDispatcherHost change is also irrelevant for this CL: it's not implemented.
Related details
- Gabriel Brito
- Guido Urdaneta
Olga Sharonova (Gerrit)
Olga Sharonova added 1 comment![]( "Open in Gerrit")
class AudioOutputDeviceSwitcher {
public:
virtual ~AudioOutputDeviceSwitcher() = default;
virtual void SwitchAudioOutputDeviceId(const std::string& device_id) = 0;
virtual GlobalRenderFrameHostId GetGlobalRenderFrameHostId() const = 0;
};Introduce this and the device switch logic from https://chromium-review.googlesource.com/c/chromium/src/+/5530803/11 in the CL *preceding* this one.
Otherwise this interface is hanging in the air without actual meaning, and we can only guess about the intentions by the fact that the device switcher was named "broker".
Sunggook Chue (Gerrit)
Sunggook Chue added 9 comments![]( "Open in Gerrit")
addressed comments based on f2f meeting, thanks.
class AudioOutputDeviceSwitcher {
public:
virtual ~AudioOutputDeviceSwitcher() = default;
virtual void SwitchAudioOutputDeviceId(const std::string& device_id) = 0;
virtual GlobalRenderFrameHostId GetGlobalRenderFrameHostId() const = 0;
};Introduce this and the device switch logic from https://chromium-review.googlesource.com/c/chromium/src/+/5530803/11 in the CL *preceding* this one.
Otherwise this interface is hanging in the air without actual meaning, and we can only guess about the intentions by the fact that the device switcher was named "broker".
Done
Follow Olga's indications on how to split the CLs, and make sure this appears in the same CL where the method is added to the mojom file and that it has a real implementation.
Done
void AddBrokerAndGetOverrideDeviceId(
AudioOutputDeviceSwitcher* device_switcher,
base::OnceCallback<void(const std::string&)> callback);
void GetOverrideDeviceId(
const int process_id,
const int render_frame_id,
base::OnceCallback<void(const std::string&)> callback);
void RemoveBrokerEntry(AudioOutputDeviceSwitcher* device_switcher);
void RemoveRenderFrameHostEntry(RenderFrameHost* render_frame_host);
I'd like to see a CL which clearly introduces usage of these interfaces. Only looking at it we can judge this one. Currently the usage is introduces in the last CL of the chain along with many other things, so we can't review effectively.
These interfaces are essential for this class, so unit tests will give idea what these methods are.
I think it is same with already added Audio Services where we implemented before its clients; bottom up approach what you suggested, instead of client first, which is my initial CLs.
If we add methods with its clients, then the class can't have any methods yet all.
Re: naming: we agreed to used Switcher; this class is unaware of brokers. Please fix everywhere.
Done
// streaming is active. The cached sink id will be used instead of the activeWhat does "streaming is active" mean?
current playing.
// forwards the request to the corresponding active audio service ifSunggook ChueWhat does it mean?
audio output stream in the audio service.
weak_ptr_factory_.GetWeakPtr(), broker,Sunggook ChueWhich thread PreferredAudioOutputDeviceManager is destroyed on?
weak_ptr can't be used on IO thread, if the PreferredAudioOutputDeviceManager is destroyed on UI thread (see base/memory/weak_ptr.h)
Olga SharonovaPreferredAudioOutputDeviceManager is created and destroyed on IO thread like VideoCaptureManager.
I don't find weak_ptr (WeakPtr) can't be on IO thread although it enforce use of same thread sequence.
PreferredAudioOutputDeviceManager lifetime belongs MediaStreamManager that also has VideoCaptureManager, which has VideoCaptureController.
VideoCaptureController is also created and destroyed like PreferredAudioOutputDeviceManager, and it is WeakPtrFactory.
Can you point out any guideline, i might missed that?
Sunggook ChuePreferredAudioOutputDeviceManager is created and destroyed on IO thread like VideoCaptureManager.
VideoCaptureManager is refcounted, so it's unknown on which thread it is destroyed. Also, it does not seem to use weak pointers?
VideoCaptureController is the one who uses weak pointers, and not necessarily correctly, since it also seems to be refcounted. I don't know what guarantees the correctness there (probably nothing), we may need ask the authors of the code.Also, media stream manager does not seem to be destroyed on IO thread, rather IO thread is destroyed before: https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/media/media_stream_manager.cc;l=1643;drc=7b232da0f22e8cdf555d43c52b6491baeb87f729;bpv=1;bpt=1
Guido knows this part better.
Guido UrdanetaVideoCaptureManager (PreferredAudioOutputDeviceManager) will be destroyed in the IO thread during DestructionObserver::WillDestroyCurrentMessageLoop and MediaStreamManager is destroyed on non IO thread after this.
Olga SharonovaLike Olga says, MediaStreamManager is **not** destroyed on the IO thread. It outlives the IO thread.
VideoCaptureManager (PreferredAudioOutputDeviceManager) will be destroyed in the IO thread during DestructionObserver::WillDestroyCurrentMessageLoop
Please point to the place in the code you refer to.
Also please document the reasoning bout the lifetime in the code. Why it's safe to use weak pointers, why the object will be destroyed on IO thread, etc.
Also please document the object threading: created on thread x, requests are forwarded to thread y, destroyed on thread z.
Please take a look at the media_stream_manager.cc changes where it instantiates and destroyed at the IO thread. (same place with VideoCaptureManager).
SetPreferredSinkId(string sink_id) => (media.mojom.OutputDeviceStatus status);Sunggook ChueWhy this is a part of this CL?
Olga SharonovaPreferredAudioOutputDeviceManager::SetPreferredSinkId function needs 'SetPreferredSinkIdCallback', which come from this.
Sunggook ChueIt does not come from this one, it comes from AudioOutputDeviceSwitcher.
Olga Sharonovaif delete it from the media_devices, the compilation error.
../..\content/browser/renderer_host/media/media_devices_dispatcher_host.h(80,27): error: unknown type name 'SetPreferredSinkIdCallback'
80 | SetPreferredSinkIdCallback callback) override;
Because MediaDispatcherHost change is also irrelevant for this CL: it's not implemented.
What I mean, PreferredAudioOutputDeviceManager::SetPreferredSinkId needs 'blink::mojom::MediaDevicesDispatcherHost::SetPreferredSinkIdCallback', which needs media_devices.mojom change.
If we want to delete this one here, then we can't implement PreferredAudioOutputDeviceManager::SetPreferredSinkId as it misses
PreferredAudioOutputDeviceManager class will be called from
1. from the broker class: 3rd CL.
2. from the renderer through MediaDispatcherHost host,
Renderer -> MediaDispatcherHost-> MediaStreamManager->PreferredAudioOutputDeviceManager.
Here, I will skip SetPreferredSinkId implementation, so,
4th CL: PreferredAudioOutputDeviceManager with custom SetPreferredSinkIdCallback
5th CL: Broker interact with PreferredAudioOutputDeviceManager.
6th CL: Device Authorization.
7th CL: MediaStreamManager instantiate PreferredAudioOutputDeviceManager and media_devices.mojom (MediaDispatcherHost), correct type of SetPreferredSinkIdCallback.
8th CL: Renderer call MediaDispatcherHost with IDL.
Related details
- Gabriel Brito
- Guido Urdaneta
- Olga Sharonova
- Steve Becker
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olga Sharonova (Gerrit)
Olga Sharonova added 6 comments![]( "Open in Gerrit")
std::map<RenderFrameHost*, WebContentsSinkId> web_contents_to_sink_ids_;Document that it's only accessed on IO thread.
void SetPreferredSinkId(const std::string& sink_id,
SetPreferredSinkIdCallback callback,
GlobalRenderFrameHostId rfh_id);
void AddSwitcherAndGetOverrideDeviceId(
AudioOutputDeviceSwitcher* device_switcher,
base::OnceCallback<void(const std::string&)> callback);
void GetOverrideDeviceId(
const int process_id,
const int render_frame_id,
base::OnceCallback<void(const std::string&)> callback);
void RemoveSwitcherEntry(AudioOutputDeviceSwitcher* device_switcher);
void RemoveRenderFrameHostEntry(RenderFrameHost* render_frame_host);Please document the API: what are the parameters, on which threads which of the method is expected to be called, etc.
base::OnceCallback<void(const std::string&)> callback);Ditto
base::OnceCallback<void(const std::string&)> callback);What is this callback, on which thread it's expected to run? Please document.
class CONTENT_EXPORT PreferredAudioOutputDeviceManager {I requested in one of the earlier CLs to document this class lifetime, which thread it's destroyed on, why weak pointers are safe, etc. - could you please do that?
Also, since the methods hop through threads all the time - please provide sequence diagrams for them.
void PreferredAudioOutputDeviceManager::RemoveSwitcherEntry(I don't think this works. This method is called in the broker destructor. When it exits, the device switcher pointer is no longer valid. However, we jump through UI thread and back to IO before actually removing it from the map. Meanwhile, there may be preferred device switch calls already scheduled on IO thread, and they will call the already deleted switcher before we get to RemoveSwitcherEntryOnIOThread().
Please see above my request for sequence diagrams. There may be other problems like that, for example when web content is gone.
Related details
- Gabriel Brito
- Guido Urdaneta
- Steve Becker
- Sunggook Chue
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sunggook Chue (Gerrit)
Sunggook Chue added 7 comments![]( "Open in Gerrit")
std::map<RenderFrameHost*, WebContentsSinkId> web_contents_to_sink_ids_;Document that it's only accessed on IO thread.
Done
void SetPreferredSinkId(const std::string& sink_id,
SetPreferredSinkIdCallback callback,
GlobalRenderFrameHostId rfh_id);
void AddSwitcherAndGetOverrideDeviceId(
AudioOutputDeviceSwitcher* device_switcher,
base::OnceCallback<void(const std::string&)> callback);
void GetOverrideDeviceId(
const int process_id,
const int render_frame_id,
base::OnceCallback<void(const std::string&)> callback);
void RemoveSwitcherEntry(AudioOutputDeviceSwitcher* device_switcher);
void RemoveRenderFrameHostEntry(RenderFrameHost* render_frame_host);Please document the API: what are the parameters, on which threads which of the method is expected to be called, etc.
Done
base::OnceCallback<void(const std::string&)> callback);Sunggook ChueDitto
Done
base::OnceCallback<void(const std::string&)> callback);What is this callback, on which thread it's expected to run? Please document.
Done
class CONTENT_EXPORT PreferredAudioOutputDeviceManager {I requested in one of the earlier CLs to document this class lifetime, which thread it's destroyed on, why weak pointers are safe, etc. - could you please do that?
Also, since the methods hop through threads all the time - please provide sequence diagrams for them.
I added comments, feel free to suggest if needed.
void PreferredAudioOutputDeviceManager::RemoveSwitcherEntry(I don't think this works. This method is called in the broker destructor. When it exits, the device switcher pointer is no longer valid. However, we jump through UI thread and back to IO before actually removing it from the map. Meanwhile, there may be preferred device switch calls already scheduled on IO thread, and they will call the already deleted switcher before we get to RemoveSwitcherEntryOnIOThread().
Please see above my request for sequence diagrams. There may be other problems like that, for example when web content is gone.
The current logic is same with 'NotifyFrameHostOfAudioStreamStopped', which is called in AudioOutputStreamBroker dtor and operate on UI thread.
All it needs the RenderFrameHost object is alive on the UI thread, and it doesn't have to live when it back to the IO thread as it is nothing but a integer.
It is called on the UI thread for WebContents, where it is even safer than broker since it doesn't have to switch to UI thread.
It's not convenient to draw sequence diagram on the /h file, anyway, tried.
Related details
- Gabriel Brito
- Guido Urdaneta
- Olga Sharonova
- Steve Becker
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olga Sharonova (Gerrit)
Olga Sharonova added 2 comments![]( "Open in Gerrit")
// substitued for the new audio output stream if the requested stream is"substitued" is a possible misspelling of "substituted" or "substitute".
Please fix.
void PreferredAudioOutputDeviceManager::RemoveSwitcherEntry(Sunggook ChueI don't think this works. This method is called in the broker destructor. When it exits, the device switcher pointer is no longer valid. However, we jump through UI thread and back to IO before actually removing it from the map. Meanwhile, there may be preferred device switch calls already scheduled on IO thread, and they will call the already deleted switcher before we get to RemoveSwitcherEntryOnIOThread().
Please see above my request for sequence diagrams. There may be other problems like that, for example when web content is gone.
The current logic is same with 'NotifyFrameHostOfAudioStreamStopped', which is called in AudioOutputStreamBroker dtor and operate on UI thread.
All it needs the RenderFrameHost object is alive on the UI thread, and it doesn't have to live when it back to the IO thread as it is nothing but a integer.
It is called on the UI thread for WebContents, where it is even safer than broker since it doesn't have to switch to UI thread.
It's not convenient to draw sequence diagram on the /h file, anyway, tried.
I mean: *a* sequence diagram - in any readable format, not in .h file. Since we have not discussed this in a design doc, and the interactions are rather complex. Also, because I just described a problem with these sequences.
I don't see how your comment addresses the situation I described though: SetPreferredSinkIdOnIOThread() being called while we jump here from IO thread to UI and back to removed a deleted switcher from the map.
Related details
- Gabriel Brito
- Guido Urdaneta
- Steve Becker
- Sunggook Chue
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olga Sharonova (Gerrit)
Olga Sharonova added 1 comment![]( "Open in Gerrit")
void PreferredAudioOutputDeviceManager::RemoveRenderFrameHostEntry(There may be SetPreferredSinkIdOnIOThread() scheduled before RemoveRenderFrameHostEntryEntryOnIOThread is called.
Olga Sharonova (Gerrit)
Olga Sharonova added 1 comment![]( "Open in Gerrit")
&GetRenderFrameHostOnUIThread,
device_switcher->GetGlobalRenderFrameHostId(),I think we should reconsider this part of PreferredAudioOutputDeviceManager. Why do we make it fetch the host, rather than providing it? For example, the host is known for the Broker aka Switcher during its creation, isn't it? So Switcher can have a getter for it, rather than for its id.
If we make PreferredAudioOutputDeviceManager to work with main frame directly, the threading will be much simpler and safer.
Am I missing something?
Sunggook Chue (Gerrit)
Sunggook Chue added 5 comments![]( "Open in Gerrit")
addressed and replied comments, thanks.
// substitued for the new audio output stream if the requested stream is"substitued" is a possible misspelling of "substituted" or "substitute".
Please fix.
Done
&GetRenderFrameHostOnUIThread,
device_switcher->GetGlobalRenderFrameHostId(),I think we should reconsider this part of PreferredAudioOutputDeviceManager. Why do we make it fetch the host, rather than providing it? For example, the host is known for the Broker aka Switcher during its creation, isn't it? So Switcher can have a getter for it, rather than for its id.
If we make PreferredAudioOutputDeviceManager to work with main frame directly, the threading will be much simpler and safer.
Am I missing something?
Actually, Broker is on the IO thread only and its knowledge of the Host is limited to its integer ID (process id, frame id), not a RenderFrameHost object.
We need to get RenderFrameHost to tell if it is main or sub, and RenderFrameHost can be acquired from the IDs only in the UI thread.
void PreferredAudioOutputDeviceManager::RemoveSwitcherEntry(Sunggook ChueI don't think this works. This method is called in the broker destructor. When it exits, the device switcher pointer is no longer valid. However, we jump through UI thread and back to IO before actually removing it from the map. Meanwhile, there may be preferred device switch calls already scheduled on IO thread, and they will call the already deleted switcher before we get to RemoveSwitcherEntryOnIOThread().
Please see above my request for sequence diagrams. There may be other problems like that, for example when web content is gone.
Olga SharonovaThe current logic is same with 'NotifyFrameHostOfAudioStreamStopped', which is called in AudioOutputStreamBroker dtor and operate on UI thread.
All it needs the RenderFrameHost object is alive on the UI thread, and it doesn't have to live when it back to the IO thread as it is nothing but a integer.
It is called on the UI thread for WebContents, where it is even safer than broker since it doesn't have to switch to UI thread.
It's not convenient to draw sequence diagram on the /h file, anyway, tried.
I mean: *a* sequence diagram - in any readable format, not in .h file. Since we have not discussed this in a design doc, and the interactions are rather complex. Also, because I just described a problem with these sequences.
I don't see how your comment addresses the situation I described though: SetPreferredSinkIdOnIOThread() being called while we jump here from IO thread to UI and back to removed a deleted switcher from the map.
That's good point, probably we will have to use WeakPtr for the store of the Switcher interface. updated.
void PreferredAudioOutputDeviceManager::RemoveRenderFrameHostEntry(There may be SetPreferredSinkIdOnIOThread() scheduled before RemoveRenderFrameHostEntryEntryOnIOThread is called.
Yes, it could be, but the worst case is that SetPreferredSinkIdOnIOThread will call the SwitchAudioOutputDeviceId of the broker, but the broker could have been called if not deleted from the list yey; The broker is a WeakPtr already that will be handled gracefully there whether the Broker is still alive or not.
Related details
- Gabriel Brito
- Guido Urdaneta
- Olga Sharonova
- Steve Becker