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
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olga Sharonova (Gerrit)
Olga Sharonova added 9 comments![]( "Open in Gerrit")
if (check_type == RenderFrameHostCheckType::kTopFrame && rfh->GetParent()) {Can be nullptr?
RenderFrameHost* render_frame_host) {Can be deleted on UI thread meanwhile, can't it?
media::OutputDeviceStatus::OUTPUT_DEVICE_STATUS_ERROR_NOT_AUTHORIZED);Any other possible reasons for it being nullptr?
DCHECK_EQ(render_frame_host->GetMainFrame(), render_frame_host);Not thread-safe?
&GetRenderFrameHostOnUIThread,
device_switcher->GetGlobalRenderFrameHostId(),Sunggook ChueI 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.
It's not set in stone, we can reconsider it/add additional information. What would be the problems if a broker holds to the host, passed in the constructor? It can also be in an anonymous form of "group id" (see the comment below).
I'm not proposing to rewrite everything immediately, but I think we should consider this options. The current design is quite complicated and not very robust to future changes; maybe that would simplify it.
RenderFrameHost* main_frame = render_frame_host->GetMainFrame();Not thread-safe?
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.
Sunggook ChueI 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.
SG. I'm still looking forward to a sequence diagram describing thread jumps. Can be added to the design doc, for example. Or be in any other readable form.
RenderFrameHost* main_frame = render_frame_host->GetMainFrame();not thread-safe?
void PreferredAudioOutputDeviceManager::RemoveRenderFrameHostEntry(Sunggook ChueThere 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.
With added weak pointers - yes.
But how about render_frame_host deleted on UI thread before/during any IO thread calls accessing it?
I'd prefer this CL treating RenderFrameHost* as an anonymous "group id" (and Switcher interface providing a getter for it). Would be much safer and simpler, and a better separation of concerns - as far as I can see.
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. |
Guido Urdaneta (Gerrit)
Guido Urdaneta added 5 comments![]( "Open in Gerrit")
std::map<RenderFrameHost*, WebContentsSinkId> web_contents_to_sink_ids_;Using raw pointers that can go dangling as keys a map looks dangerous to me.
Can you replace it with the global routing IDs?
// The active streaming device_switchers that are using the system defaultIs it always system default, or could it be a non-system-default device if the user has called SetPreferredSinkId?
struct WebContentsSinkId {Given the restrictions on how this is created (only default or move constructor), it looks like this should be a class.
Also, if you define move constructor, you should also define move assignment.
// substitued for the new audio output stream if the requested stream is"substitued" is a possible misspelling of "substituted" or "substitute".
Please fix.
- Gabriel Brito
- 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 14 comments![]( "Open in Gerrit")
std::map<RenderFrameHost*, WebContentsSinkId> web_contents_to_sink_ids_;Using raw pointers that can go dangling as keys a map looks dangerous to me.
Can you replace it with the global routing IDs?
global routing id can't give any information of the called render frame host (RFH)'s relation to its top RFH or children RFH.
for example, RFH_A (top), RFH_B(top) are cached entries. And when RHF_C (child) is called, it should tell which RFH RHF_C child of? global ID can't help.
// The active streaming device_switchers that are using the system defaultIs it always system default, or could it be a non-system-default device if the user has called SetPreferredSinkId?
it should be system default regardless of SetprefferedSinkId device id for switchers.
Switchers is the one that user listen to audio device that is a system default or device from setSinkId. If it isn't system default device id from setSinkId, we don't register it (not called for registration).
Given the restrictions on how this is created (only default or move constructor), it looks like this should be a class.
Also, if you define move constructor, you should also define move assignment.
Done
// substitued for the new audio output stream if the requested stream is"substitued" is a possible misspelling of "substituted" or "substitute".
Please fix.
Done
if (check_type == RenderFrameHostCheckType::kTopFrame && rfh->GetParent()) {Sunggook ChueCan be nullptr?
Done
Can be deleted on UI thread meanwhile, can't it?
it could be, but it doesn't matter because it is just int_ptr type for key, no object access.
media::OutputDeviceStatus::OUTPUT_DEVICE_STATUS_ERROR_NOT_AUTHORIZED);Any other possible reasons for it being nullptr?
It is common pattern of RenderFrameHost::FromID where the caller ensure it returns nullptr. I'm not expert in that area, but I assume that the frame host process or frame could be terminated.
DCHECK_EQ(render_frame_host->GetMainFrame(), render_frame_host);Sunggook ChueNot thread-safe?
no, GetMainFrame is one of thread agnostic methods in the RenderFrameHostImpl.
&GetRenderFrameHostOnUIThread,
device_switcher->GetGlobalRenderFrameHostId(),Sunggook ChueI 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?
Olga SharonovaActually, 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.
It's not set in stone, we can reconsider it/add additional information. What would be the problems if a broker holds to the host, passed in the constructor? It can also be in an anonymous form of "group id" (see the comment below).
I'm not proposing to rewrite everything immediately, but I think we should consider this options. The current design is quite complicated and not very robust to future changes; maybe that would simplify it.
The client who passed the host should do same thing as the client is in the IO thread: IOThread -> UIThread -> IOThread.
https://docs.google.com/document/d/1xVn0VwLa8w7jwXl6aeOfYn0mYK7zEc4pSmHelHAbi2g/edit?usp=sharing
"The Browser has to cache the default sink id for the main RenderFrameHost, but the input is process id and frame id so it has to retrieve RenderFrameHost on the UI thread after being called on the IO thread. Once it retrieves RenderFrameHost on the UI thread, it has to return to the IO thread in order to call the Switcher. The logic seems too complicated, can we find simplified file solution here."
RenderFrameHost* main_frame = render_frame_host->GetMainFrame();Sunggook ChueNot thread-safe?
ditto above, multi threads method.
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.
Sunggook ChueI 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.
Olga SharonovaThat's good point, probably we will have to use WeakPtr for the store of the Switcher interface. updated.
SG. I'm still looking forward to a sequence diagram describing thread jumps. Can be added to the design doc, for example. Or be in any other readable form.
I've added it 'Sequence'. https://chromium-review.googlesource.com/c/chromium/src/+/5664173/9?tab=comments
RenderFrameHost* main_frame = render_frame_host->GetMainFrame();Sunggook Chuenot thread-safe?
ditto,
void PreferredAudioOutputDeviceManager::RemoveRenderFrameHostEntry(Sunggook ChueThere may be SetPreferredSinkIdOnIOThread() scheduled before RemoveRenderFrameHostEntryEntryOnIOThread is called.
Olga SharonovaYes, 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.
With added weak pointers - yes.
But how about render_frame_host deleted on UI thread before/during any IO thread calls accessing it?
I'd prefer this CL treating RenderFrameHost* as an anonymous "group id" (and Switcher interface providing a getter for it). Would be much safer and simpler, and a better separation of concerns - as far as I can see.
When RFH (RenderFrameHost) is before/deleted in the during of IO thread calls, it doesn't affect the call as it doesn't access the object on the IO thread, and WeakPtr for Switcher also protect the Broker pointer access.
I really wish we don't have to use RenderFrameHost, but unfortunately we need to find a main RenderFrameHost whether the requested host is related to the called setDefaultSinkId. But there is no way at this time to figure out whether 2 different RenderFrameHostId belongs to same main RenderFrameHost. I added your proposal under the open issues.
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. |
Sunggook Chue (Gerrit)
Sunggook Chue added 1 comment![]( "Open in Gerrit")
tl...@chromium.orgOlga, lease ignore this file as I edited to avoid upload issue that has been issue last couple of days. I will remove this one before landing, or have Lei https://chromium-review.googlesource.com/c/chromium/src/+/5735589 reviewed this file.
Related details
- Gabriel Brito
- Guido Urdaneta
- 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. |
Sunggook Chue (Gerrit)
Sunggook Chue added 1 comment![]( "Open in Gerrit")
Olga, lease ignore this file as I edited to avoid upload issue that has been issue last couple of days. I will remove this one before landing, or have Lei https://chromium-review.googlesource.com/c/chromium/src/+/5735589 reviewed this file.
Done
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. |
Guido Urdaneta (Gerrit)
Guido Urdaneta added 1 comment![]( "Open in Gerrit")
moving myself to cc since Olga is the main owner here and her review should be enough.
Related details
- Gabriel Brito
- Olga Sharonova
- Steve Becker
- Sunggook Chue
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")
Please do not mark the comments as resolved until the consensus is reached. Looks like there are several discussions lost in the resolved comments.
Also, I explained before: since there is no detailed design, I can't review the class without seeing the context, so a follow-up CL is needed.
I remember I asked for a sequence diagram, it's probably somewhere in the resolved comments - could you point to it? Thanks!
RenderFrameHost* render_frame_host) {Sunggook ChueCan be deleted on UI thread meanwhile, can't it?
it could be, but it doesn't matter because it is just int_ptr type for key, no object access.
There is a data race: below: render_frame_host->GetMainFrame()
DCHECK_EQ(render_frame_host->GetMainFrame(), render_frame_host);Sunggook ChueNot thread-safe?
no, GetMainFrame is one of thread agnostic methods in the RenderFrameHostImpl.
See above: looks like a data race. (Also we can't conclude that the API is thread agnostic base on one of implementations.)
device_switcher->GetGlobalRenderFrameHostId();device_switcher is a WeakPtr => can be null.
RenderFrameHost* main_frame = render_frame_host->GetMainFrame();Sunggook ChueNot thread-safe?
ditto above, multi threads method.
Same here: data race accessing render_frame_host?
device_switcher->GetGlobalRenderFrameHostId();Here and in other places: device_switcher is a WeakPtr => can be null.
RenderFrameHost* main_frame = render_frame_host->GetMainFrame();Sunggook Chuenot thread-safe?
ditto,
same
void PreferredAudioOutputDeviceManager::RemoveRenderFrameHostEntry(Sunggook ChueThere may be SetPreferredSinkIdOnIOThread() scheduled before RemoveRenderFrameHostEntryEntryOnIOThread is called.
Olga SharonovaYes, 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.
Sunggook ChueWith added weak pointers - yes.
But how about render_frame_host deleted on UI thread before/during any IO thread calls accessing it?
I'd prefer this CL treating RenderFrameHost* as an anonymous "group id" (and Switcher interface providing a getter for it). Would be much safer and simpler, and a better separation of concerns - as far as I can see.
When RFH (RenderFrameHost) is before/deleted in the during of IO thread calls, it doesn't affect the call as it doesn't access the object on the IO thread, and WeakPtr for Switcher also protect the Broker pointer access.
I really wish we don't have to use RenderFrameHost, but unfortunately we need to find a main RenderFrameHost whether the requested host is related to the called setDefaultSinkId. But there is no way at this time to figure out whether 2 different RenderFrameHostId belongs to same main RenderFrameHost. I added your proposal under the open issues.
I'm not sure we are talking about the same problem. This code is calling methods of raw pointer (render_frame_host) while it may be being deleted on UI thread.
Related details
- Gabriel Brito
- 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 4 comments![]( "Open in Gerrit")
I still think it would probably be more effective to discuss the design of this class: where it lives, where its dependencies live, how all their lifetimes are managed, what is accessed on which threads, etc; rather than doing massive iterations on the source code.
std::map<RenderFrameHost*, WebContentsSinkId> web_contents_to_sink_ids_;Why do we mention web contents here? Let's keep consistent terminoligy.
std::map<RenderFrameHost*, WebContentsSinkId> web_contents_to_sink_ids_;Sunggook ChueUsing raw pointers that can go dangling as keys a map looks dangerous to me.
Can you replace it with the global routing IDs?
global routing id can't give any information of the called render frame host (RFH)'s relation to its top RFH or children RFH.
for example, RFH_A (top), RFH_B(top) are cached entries. And when RHF_C (child) is called, it should tell which RFH RHF_C child of? global ID can't help.
There is a way to get RFH from global ID. However, RenderFrameHost::FromID can only be called on the UI thread.
Looks like this CL is using a raw pointer to RHF in a non-threadsafe manner.
Why have you decided to make it PreferredAudioOutputDeviceManager's responsibility to traverse the tree? Why can't it always accept only the global id of the main frame and live peacefully on IO thread, and let the caller take care of finding that id?
void PreferredAudioOutputDeviceManager::RemoveRenderFrameHostEntry(Sunggook ChueThere may be SetPreferredSinkIdOnIOThread() scheduled before RemoveRenderFrameHostEntryEntryOnIOThread is called.
Olga SharonovaYes, 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.
Sunggook ChueWith added weak pointers - yes.
But how about render_frame_host deleted on UI thread before/during any IO thread calls accessing it?
I'd prefer this CL treating RenderFrameHost* as an anonymous "group id" (and Switcher interface providing a getter for it). Would be much safer and simpler, and a better separation of concerns - as far as I can see.
Olga SharonovaWhen RFH (RenderFrameHost) is before/deleted in the during of IO thread calls, it doesn't affect the call as it doesn't access the object on the IO thread, and WeakPtr for Switcher also protect the Broker pointer access.
I really wish we don't have to use RenderFrameHost, but unfortunately we need to find a main RenderFrameHost whether the requested host is related to the called setDefaultSinkId. But there is no way at this time to figure out whether 2 different RenderFrameHostId belongs to same main RenderFrameHost. I added your proposal under the open issues.
I'm not sure we are talking about the same problem. This code is calling methods of raw pointer (render_frame_host) while it may be being deleted on UI thread.
- 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. |
Guido Urdaneta (Gerrit)
Guido Urdaneta added 1 comment![]( "Open in Gerrit")
std::map<RenderFrameHost*, WebContentsSinkId> web_contents_to_sink_ids_;Sunggook ChueUsing raw pointers that can go dangling as keys a map looks dangerous to me.
Can you replace it with the global routing IDs?
Olga Sharonovaglobal routing id can't give any information of the called render frame host (RFH)'s relation to its top RFH or children RFH.
for example, RFH_A (top), RFH_B(top) are cached entries. And when RHF_C (child) is called, it should tell which RFH RHF_C child of? global ID can't help.
There is a way to get RFH from global ID. However, RenderFrameHost::FromID can only be called on the UI thread.
Looks like this CL is using a raw pointer to RHF in a non-threadsafe manner.
Why have you decided to make it PreferredAudioOutputDeviceManager's responsibility to traverse the tree? Why can't it always accept only the global id of the main frame and live peacefully on IO thread, and let the caller take care of finding that id?
raw pointers can become dangling or can be reused if the originals are destroyed and new objects created.
There is a 1:1 mapping between routing IDs and RFHs (https://crsrc.org/c/content/browser/renderer_host/render_frame_host_impl.h;l=347) and you can't access RFHs outside the main thread, so it's better to use routing IDs (which don't have the issues raw pointers have) outside of the main thread.
Related details
- Gabriel Brito
- 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. |
Sunggook Chue (Gerrit)
Sunggook Chue added 10 comments![]( "Open in Gerrit")
I still think it would probably be more effective to discuss the design of this class: where it lives, where its dependencies live, how all their lifetimes are managed, what is accessed on which threads, etc; rather than doing massive iterations on the source code.
I believe I've explained few times but it looks like it isn't perfect or we are lost during conversation, basically it is same with VideoCaptureManager in terms of lifetime and its threading model if you're familiar with media components in the browser.
Anyway, I've added threading model and dependency on the design doc. Please leave a comments if need more clarification.
https://docs.google.com/document/d/1xVn0VwLa8w7jwXl6aeOfYn0mYK7zEc4pSmHelHAbi2g/edit?usp=sharing
std::map<RenderFrameHost*, WebContentsSinkId> web_contents_to_sink_ids_;Why do we mention web contents here? Let's keep consistent terminoligy.
it's good point, updated it.
std::map<RenderFrameHost*, WebContentsSinkId> web_contents_to_sink_ids_;Sunggook ChueUsing raw pointers that can go dangling as keys a map looks dangerous to me.
Can you replace it with the global routing IDs?
Olga Sharonovaglobal routing id can't give any information of the called render frame host (RFH)'s relation to its top RFH or children RFH.
for example, RFH_A (top), RFH_B(top) are cached entries. And when RHF_C (child) is called, it should tell which RFH RHF_C child of? global ID can't help.
Guido Urdaneta
There is a way to get RFH from global ID. However, RenderFrameHost::FromID can only be called on the UI thread.
Looks like this CL is using a raw pointer to RHF in a non-threadsafe manner.
Why have you decided to make it PreferredAudioOutputDeviceManager's responsibility to traverse the tree? Why can't it always accept only the global id of the main frame and live peacefully on IO thread, and let the caller take care of finding that id?
raw pointers can become dangling or can be reused if the originals are destroyed and new objects created.
There is a 1:1 mapping between routing IDs and RFHs (https://crsrc.org/c/content/browser/renderer_host/render_frame_host_impl.h;l=347) and you can't access RFHs outside the main thread, so it's better to use routing IDs (which don't have the issues raw pointers have) outside of the main thread.
The problem is that we have to figure out the parent and child relationship. for example, the parent RFH calls SetPreferredSinkId, then its all sub frames will use preferred sink id. we can't figure out these parent/child relationship with routing IDs. If there is a way for it, please let me know. I really want to go that path.
RenderFrameHost* render_frame_host) {Sunggook ChueCan be deleted on UI thread meanwhile, can't it?
Olga Sharonovait could be, but it doesn't matter because it is just int_ptr type for key, no object access.
There is a data race: below: render_frame_host->GetMainFrame()
updated based on the above feedback. thanks.
DCHECK_EQ(render_frame_host->GetMainFrame(), render_frame_host);Sunggook ChueNot thread-safe?
Olga Sharonovano, GetMainFrame is one of thread agnostic methods in the RenderFrameHostImpl.
See above: looks like a data race. (Also we can't conclude that the API is thread agnostic base on one of implementations.)
Done
device_switcher is a WeakPtr => can be null.
It is direct call from the device_switcher, so leave it as it is so that we can capture the invalid call here, instead add hardening in the IO thread. thanks.
RenderFrameHost* main_frame = render_frame_host->GetMainFrame();Sunggook ChueNot thread-safe?
Olga Sharonovaditto above, multi threads method.
Same here: data race accessing render_frame_host?
actually GetMainFrame can be accessed from any threads, but I've removed this one from the IO thread based on other feedback.
Here and in other places: device_switcher is a WeakPtr => can be null.
will add CHECK here as it should be called from the AudioOutputDeviceSwitcher object.
RenderFrameHost* main_frame = render_frame_host->GetMainFrame();Sunggook Chuenot thread-safe?
Olga Sharonovaditto,
same
Done
void PreferredAudioOutputDeviceManager::RemoveRenderFrameHostEntry(Sunggook ChueThere may be SetPreferredSinkIdOnIOThread() scheduled before RemoveRenderFrameHostEntryEntryOnIOThread is called.
Olga SharonovaYes, 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.
Sunggook ChueWith added weak pointers - yes.
But how about render_frame_host deleted on UI thread before/during any IO thread calls accessing it?
I'd prefer this CL treating RenderFrameHost* as an anonymous "group id" (and Switcher interface providing a getter for it). Would be much safer and simpler, and a better separation of concerns - as far as I can see.
Olga SharonovaWhen RFH (RenderFrameHost) is before/deleted in the during of IO thread calls, it doesn't affect the call as it doesn't access the object on the IO thread, and WeakPtr for Switcher also protect the Broker pointer access.
I really wish we don't have to use RenderFrameHost, but unfortunately we need to find a main RenderFrameHost whether the requested host is related to the called setDefaultSinkId. But there is no way at this time to figure out whether 2 different RenderFrameHostId belongs to same main RenderFrameHost. I added your proposal under the open issues.
Olga SharonovaI'm not sure we are talking about the same problem. This code is calling methods of raw pointer (render_frame_host) while it may be being deleted on UI thread.
*may have been deleted
We should not access RenderFrameHost's methods in the IO thread, instead just use it. updated the code for that. thanks.
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 10 comments![]( "Open in Gerrit")
Sunggook ChueI still think it would probably be more effective to discuss the design of this class: where it lives, where its dependencies live, how all their lifetimes are managed, what is accessed on which threads, etc; rather than doing massive iterations on the source code.
I believe I've explained few times but it looks like it isn't perfect or we are lost during conversation, basically it is same with VideoCaptureManager in terms of lifetime and its threading model if you're familiar with media components in the browser.
Anyway, I've added threading model and dependency on the design doc. Please leave a comments if need more clarification.
https://docs.google.com/document/d/1xVn0VwLa8w7jwXl6aeOfYn0mYK7zEc4pSmHelHAbi2g/edit?usp=sharing
Thanks. This reply was also almost lost: as I asked earlier, please do not resolve the comment until we reach a consensus. It's hard to dig up the resolved comments. "Similar to something" is not helpful when looking at the new class.
#include "content/browser/media/audio_output_stream_broker.h"There should be no direct dependency on output stream broker: the idea of the switcher interface is to decouple them.
For that purpose the interface needs to be declared in this CL, and implemented in the broker in the next CL.
RenderFrameHost* GetRenderFrameHostOnUIThread(GetMain... or something?
RenderFrameHost* main_frame) {Hmm. Why pointers rather than GlobalRenderFrameHostId everywhere? Haven't we just agreed on that?
device_switcher->GetGlobalRenderFrameHostId();Sunggook Chuedevice_switcher is a WeakPtr => can be null.
It is direct call from the device_switcher, so leave it as it is so that we can capture the invalid call here, instead add hardening in the IO thread. thanks.
Then better to pass it as a reference and have a AsWeakPtr() method on the switcher.
&GetRenderFrameHostOnUIThread, rfh_id,I think I asked it before, but not sure (please don't resolve the comment, otherwise the answer will get lost): have you looked into the option of determining the main host id on the broker creation?
AddSwitcherAndGetOverrideDeviceIdOnIOThread(We need to rename these types of methods: "OnThread" naming is used if the original method is called on thread X, and it's trampolined to thread Y. In our case it starts already on IO thread.
RenderFrameHost* main_frame = render_frame_host->GetMainFrame();Sunggook ChueNot thread-safe?
Olga Sharonovaditto above, multi threads method.
Sunggook ChueSame here: data race accessing render_frame_host?
actually GetMainFrame can be accessed from any threads, but I've removed this one from the IO thread based on other feedback.
As I said, the race was accessing *render_frame_host pointer*, not anything in its method.
device_switcher->GetGlobalRenderFrameHostId();Sunggook ChueHere and in other places: device_switcher is a WeakPtr => can be null.
will add CHECK here as it should be called from the AudioOutputDeviceSwitcher object.
What is the point of calling it with the weak pointer?
// We don't check device_switcher is valid or not during RemoveSwitcher
// because we just use it as a interger key value for cleanup.Hmm. Weak pointers will be nulled on the switcher deletion. So it2->get() == device_switcher.get()) is comparing nullptr with nullptr, and potentially deleting some other switcher. This does not add up.
Related details
- 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")
&GetRenderFrameHostOnUIThread, rfh_id,I think I asked it before, but not sure (please don't resolve the comment, otherwise the answer will get lost): have you looked into the option of determining the main host id on the broker creation?
See RenderFrameAudioOutputStreamFactory constructor.
Sunggook Chue (Gerrit)
Sunggook Chue added 9 comments![]( "Open in Gerrit")
#include "content/browser/media/audio_output_stream_broker.h"There should be no direct dependency on output stream broker: the idea of the switcher interface is to decouple them.
For that purpose the interface needs to be declared in this CL, and implemented in the broker in the next CL.
AudioOutputDeviceSwitcher is declared in the media/audio_output_stream_broker.h.
I will move declare AudioOutputDeviceSwitcher in the preferred_audio_output_device_manager.h.
RenderFrameHost* GetRenderFrameHostOnUIThread(Sunggook ChueGetMain... or something?
Done
Hmm. Why pointers rather than GlobalRenderFrameHostId everywhere? Haven't we just agreed on that?
We don't have to use GlobalRenderFrameHostId as RenderFrameHost* is just integer as a key.
It is simpler even when it is called from the MediaWebContentsObserver where RenderFrameHost*.
device_switcher->GetGlobalRenderFrameHostId();Sunggook Chuedevice_switcher is a WeakPtr => can be null.
Olga SharonovaIt is direct call from the device_switcher, so leave it as it is so that we can capture the invalid call here, instead add hardening in the IO thread. thanks.
Then better to pass it as a reference and have a AsWeakPtr() method on the switcher.
sounds good.
Olga SharonovaI think I asked it before, but not sure (please don't resolve the comment, otherwise the answer will get lost): have you looked into the option of determining the main host id on the broker creation?
See RenderFrameAudioOutputStreamFactory constructor.
It looks like the object is called from the UI thread object RenderFrameHostImpl, meanwhile AudioOutputSreamBreaker, which call PreferredAudioOutputDeviceManager, is called on IO thread.
Afaik, most of media components (video/audio) that hooks btw Renderer and Utility Services are on IO thread bound.
If needed, we can ask Guido to review this CL (and browser side changes) if needed as he is well aware of it.
We need to rename these types of methods: "OnThread" naming is used if the original method is called on thread X, and it's trampolined to thread Y. In our case it starts already on IO thread.
that make sense, how about 'WithMainFrame'.
device_switcher->GetGlobalRenderFrameHostId();Sunggook ChueHere and in other places: device_switcher is a WeakPtr => can be null.
Olga Sharonovawill add CHECK here as it should be called from the AudioOutputDeviceSwitcher object.
What is the point of calling it with the weak pointer?
To ensure that its object that implements AudioOutputDeviceSwitcher can be deleted before it is called like a BindOnce based callback.
// We don't check device_switcher is valid or not during RemoveSwitcher
// because we just use it as a interger key value for cleanup.Hmm. Weak pointers will be nulled on the switcher deletion. So it2->get() == device_switcher.get()) is comparing nullptr with nullptr, and potentially deleting some other switcher. This does not add up.
The it2 is collection that is results of the 'main_frame' so it is find to delete any value when 'it2->get() == nullptr and device_switcher.get() is nullptr even if internal it2->get() could be different Switcher from device_switcher.
I will add comments and test to validate it.
Related details
- Olga Sharonova
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sunggook Chue (Gerrit)
Sunggook Chue added 1 comment![]( "Open in Gerrit")
RenderFrameHost* main_frame) {Sunggook ChueHmm. Why pointers rather than GlobalRenderFrameHostId everywhere? Haven't we just agreed on that?
We don't have to use GlobalRenderFrameHostId as RenderFrameHost* is just integer as a key.
It is simpler even when it is called from the MediaWebContentsObserver where RenderFrameHost*.
rethink and using GlobalId as RenderFrameHost could mean UI thread.
Olga Sharonova (Gerrit)
Olga Sharonova added 2 comments![]( "Open in Gerrit")
&GetRenderFrameHostOnUIThread, rfh_id,Olga SharonovaI think I asked it before, but not sure (please don't resolve the comment, otherwise the answer will get lost): have you looked into the option of determining the main host id on the broker creation?
Sunggook ChueSee RenderFrameAudioOutputStreamFactory constructor.
It looks like the object is called from the UI thread object RenderFrameHostImpl, meanwhile AudioOutputSreamBreaker, which call PreferredAudioOutputDeviceManager, is called on IO thread.
Afaik, most of media components (video/audio) that hooks btw Renderer and Utility Services are on IO thread bound.
If needed, we can ask Guido to review this CL (and browser side changes) if needed as he is well aware of it.
Sunggook, when you reply to the comments, there's a checkbox "resolved". I just asked you in the comment above **to not resolve the discussions in progress**. Please **do not mark the checkbox** for any ongoing discussion or when you are answering a question. I'll mark it as resolved when I've read an answer and have not follow-up questions. Resolved comments are getting lost. This makes my job as a reviewer unnecessary hard. And it causes situations when I have to ask the same question again.
---
If you look at RenderFrameAudioOutputStreamFactory constructor (running on UI thread) you see it's constructing Core. Either RenderFrameAudioOutputStreamFactory
constructor or Core constructor can get the main frame id, since the construction is happening on UI thread. The rest of the Core lives on IO thread and can pass the main frame ID down the line (here https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc;l=323;drc=82dff63dbf9db05e9274e11d9128af7b9f51ceaa;bpv=1;bpt=1, etc); or IO thread code can just access it trough an IO thread getter at some point (to not plumb the same id through too many layers).
I do not suggest to rewrite the code immediately, but I'm asking if this has been considered and why the current approach of dynamic fetch of the main frame id was chosen over an option of having a constant main frame id propagated to an IO thread object during construction.
// We don't check device_switcher is valid or not during RemoveSwitcher
// because we just use it as a interger key value for cleanup.Sunggook ChueHmm. Weak pointers will be nulled on the switcher deletion. So it2->get() == device_switcher.get()) is comparing nullptr with nullptr, and potentially deleting some other switcher. This does not add up.
The it2 is collection that is results of the 'main_frame' so it is find to delete any value when 'it2->get() == nullptr and device_switcher.get() is nullptr even if internal it2->get() could be different Switcher from device_switcher.
I will add comments and test to validate it.
I'd like to keep the discussion open, since I'm not entirely happy with how it's turning out. but have not figured an alternative proposal yet.
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 3 comments![]( "Open in Gerrit")
&GetRenderFrameHostOnUIThread, rfh_id,Olga SharonovaI think I asked it before, but not sure (please don't resolve the comment, otherwise the answer will get lost): have you looked into the option of determining the main host id on the broker creation?
Sunggook ChueSee RenderFrameAudioOutputStreamFactory constructor.
Olga SharonovaIt looks like the object is called from the UI thread object RenderFrameHostImpl, meanwhile AudioOutputSreamBreaker, which call PreferredAudioOutputDeviceManager, is called on IO thread.
Afaik, most of media components (video/audio) that hooks btw Renderer and Utility Services are on IO thread bound.
If needed, we can ask Guido to review this CL (and browser side changes) if needed as he is well aware of it.
Sunggook, when you reply to the comments, there's a checkbox "resolved". I just asked you in the comment above **to not resolve the discussions in progress**. Please **do not mark the checkbox** for any ongoing discussion or when you are answering a question. I'll mark it as resolved when I've read an answer and have not follow-up questions. Resolved comments are getting lost. This makes my job as a reviewer unnecessary hard. And it causes situations when I have to ask the same question again.
---
If you look at RenderFrameAudioOutputStreamFactory constructor (running on UI thread) you see it's constructing Core. Either RenderFrameAudioOutputStreamFactory
constructor or Core constructor can get the main frame id, since the construction is happening on UI thread. The rest of the Core lives on IO thread and can pass the main frame ID down the line (here https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc;l=323;drc=82dff63dbf9db05e9274e11d9128af7b9f51ceaa;bpv=1;bpt=1, etc); or IO thread code can just access it trough an IO thread getter at some point (to not plumb the same id through too many layers).I do not suggest to rewrite the code immediately, but I'm asking if this has been considered and why the current approach of dynamic fetch of the main frame id was chosen over an option of having a constant main frame id propagated to an IO thread object during construction.
I got your point.
So, we cache global Ids and its parent Ids (if it is sub frame) on IO thread through RenderFrameAudioOutputStreamFactory mechanism and retrieve the main frame global id without switching to UI thread.
The problem is that we need RenderFrameHostImpl object when called on the IO thread with globalId (most Media APIs is called on IO thread from the Renderer) in order to get the RenderFrameAudioOutputStreamFactory object. So switching to UI thread occurs.
Anything I missed?
// We don't check device_switcher is valid or not during RemoveSwitcher
// because we just use it as a interger key value for cleanup.Sunggook ChueHmm. Weak pointers will be nulled on the switcher deletion. So it2->get() == device_switcher.get()) is comparing nullptr with nullptr, and potentially deleting some other switcher. This does not add up.
Olga SharonovaThe it2 is collection that is results of the 'main_frame' so it is find to delete any value when 'it2->get() == nullptr and device_switcher.get() is nullptr even if internal it2->get() could be different Switcher from device_switcher.
I will add comments and test to validate it.
I'd like to keep the discussion open, since I'm not entirely happy with how it's turning out. but have not figured an alternative proposal yet.
I will file a bug once it lands as the feature flag is disabled. We can't keep this CL open.
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 2 comments![]( "Open in Gerrit")
&GetRenderFrameHostOnUIThread, rfh_id,Olga SharonovaI think I asked it before, but not sure (please don't resolve the comment, otherwise the answer will get lost): have you looked into the option of determining the main host id on the broker creation?
Sunggook ChueSee RenderFrameAudioOutputStreamFactory constructor.
Olga SharonovaIt looks like the object is called from the UI thread object RenderFrameHostImpl, meanwhile AudioOutputSreamBreaker, which call PreferredAudioOutputDeviceManager, is called on IO thread.
Afaik, most of media components (video/audio) that hooks btw Renderer and Utility Services are on IO thread bound.
If needed, we can ask Guido to review this CL (and browser side changes) if needed as he is well aware of it.
Sunggook ChueSunggook, when you reply to the comments, there's a checkbox "resolved". I just asked you in the comment above **to not resolve the discussions in progress**. Please **do not mark the checkbox** for any ongoing discussion or when you are answering a question. I'll mark it as resolved when I've read an answer and have not follow-up questions. Resolved comments are getting lost. This makes my job as a reviewer unnecessary hard. And it causes situations when I have to ask the same question again.
---
If you look at RenderFrameAudioOutputStreamFactory constructor (running on UI thread) you see it's constructing Core. Either RenderFrameAudioOutputStreamFactory
constructor or Core constructor can get the main frame id, since the construction is happening on UI thread. The rest of the Core lives on IO thread and can pass the main frame ID down the line (here https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc;l=323;drc=82dff63dbf9db05e9274e11d9128af7b9f51ceaa;bpv=1;bpt=1, etc); or IO thread code can just access it trough an IO thread getter at some point (to not plumb the same id through too many layers).I do not suggest to rewrite the code immediately, but I'm asking if this has been considered and why the current approach of dynamic fetch of the main frame id was chosen over an option of having a constant main frame id propagated to an IO thread object during construction.
I got your point.
So, we cache global Ids and its parent Ids (if it is sub frame) on IO thread through RenderFrameAudioOutputStreamFactory mechanism and retrieve the main frame global id without switching to UI thread.
The problem is that we need RenderFrameHostImpl object when called on the IO thread with globalId (most Media APIs is called on IO thread from the Renderer) in order to get the RenderFrameAudioOutputStreamFactory object. So switching to UI thread occurs.
Anything I missed?
.
// We don't check device_switcher is valid or not during RemoveSwitcher
// because we just use it as a interger key value for cleanup.Sunggook ChueHmm. Weak pointers will be nulled on the switcher deletion. So it2->get() == device_switcher.get()) is comparing nullptr with nullptr, and potentially deleting some other switcher. This does not add up.
Olga SharonovaThe it2 is collection that is results of the 'main_frame' so it is find to delete any value when 'it2->get() == nullptr and device_switcher.get() is nullptr even if internal it2->get() could be different Switcher from device_switcher.
I will add comments and test to validate it.
Sunggook ChueI'd like to keep the discussion open, since I'm not entirely happy with how it's turning out. but have not figured an alternative proposal yet.
I will file a bug once it lands as the feature flag is disabled. We can't keep this CL open.
- 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. |
Olga Sharonova (Gerrit)
1) Could you explain why have you resolved the comment? Especially since it seems like you are asking a question.
2) I mean we cache the main frame global id.
Could you clarify "the problem"? What code are you referring to? (links, methods names?) When I described my idea, I added specific references. Could you do the same?
// We don't check device_switcher is valid or not during RemoveSwitcher
// because we just use it as a interger key value for cleanup.Sunggook ChueHmm. Weak pointers will be nulled on the switcher deletion. So it2->get() == device_switcher.get()) is comparing nullptr with nullptr, and potentially deleting some other switcher. This does not add up.
Olga SharonovaThe it2 is collection that is results of the 'main_frame' so it is find to delete any value when 'it2->get() == nullptr and device_switcher.get() is nullptr even if internal it2->get() could be different Switcher from device_switcher.
I will add comments and test to validate it.
Sunggook ChueI'd like to keep the discussion open, since I'm not entirely happy with how it's turning out. but have not figured an alternative proposal yet.
Olga SharonovaI will file a bug once it lands as the feature flag is disabled. We can't keep this CL open.
.
1) It was your choice to skip design discussions and iterate on the code instead. Figuring out a complex design without a clearly communicated high level vision is never easy ant takes time and many back and forth.
2) If you think I have any other agenda besides landing easy-to-maintain code of good quality - you are mistaken. But we cannot land it just for the sake of landing it, because it creates maintenance burden. As you noticed, the review has already uncovered multiple issues, including race conditions.
We need to fix the design issues before it lands.
3) The inconsistency of weak pointer treatment hints to a design issue. You can figure it out and fix it. Or I'll think about it when I have a bit of time.
4) The fact that you keep resolving in-progress discussions when I explicitly asked you not to, because it's very hard to keep track of them - I don't even know what to make of it. What's your goal?
Sunggook Chue (Gerrit)
Sunggook Chue added 3 comments![]( "Open in Gerrit")
1. We have to access RenderFrameAudioOutputStreamFactory, which is a member of RenderFrameHost, which is accessed from only in the UI thread.
2. RenderFrameAudioOutputStreamFactory is created for every frames but preferred manager is accessed only when audio output request is made. If we cache the values for RenderFrameAudioOutputStreamFactory creation, it would increase the unnecessary cache size.
// We don't check device_switcher is valid or not during RemoveSwitcher
// because we just use it as a interger key value for cleanup.Sunggook ChueHmm. Weak pointers will be nulled on the switcher deletion. So it2->get() == device_switcher.get()) is comparing nullptr with nullptr, and potentially deleting some other switcher. This does not add up.
Olga SharonovaThe it2 is collection that is results of the 'main_frame' so it is find to delete any value when 'it2->get() == nullptr and device_switcher.get() is nullptr even if internal it2->get() could be different Switcher from device_switcher.
I will add comments and test to validate it.
Sunggook ChueI'd like to keep the discussion open, since I'm not entirely happy with how it's turning out. but have not figured an alternative proposal yet.
Olga SharonovaI will file a bug once it lands as the feature flag is disabled. We can't keep this CL open.
Olga Sharonova.
1) It was your choice to skip design discussions and iterate on the code instead. Figuring out a complex design without a clearly communicated high level vision is never easy ant takes time and many back and forth.
2) If you think I have any other agenda besides landing easy-to-maintain code of good quality - you are mistaken. But we cannot land it just for the sake of landing it, because it creates maintenance burden. As you noticed, the review has already uncovered multiple issues, including race conditions.
We need to fix the design issues before it lands.3) The inconsistency of weak pointer treatment hints to a design issue. You can figure it out and fix it. Or I'll think about it when I have a bit of time.
4) The fact that you keep resolving in-progress discussions when I explicitly asked you not to, because it's very hard to keep track of them - I don't even know what to make of it. What's your goal?
I already mentioned my design, please let me know what's your proposal. I don't see any design issue.
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 1 comment![]( "Open in Gerrit")
This is not answering my question though. What code are you referring to? Who "we"? When we have to access RenderFrameAudioOutputStreamFactory? What exactly are you talking about? Which operation?
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. |
Gabriel Brito (Gerrit)
Gabriel Brito added 2 comments![]( "Open in Gerrit")
Please let me try to aid the discussion 😊
Olga, I think that what Sunggook is trying to say is that his mechanism of dynamically posting a `GetMainGlobalIdOnUIThread` task on the UI thread during `PreferredAudioOutputDeviceManager::AddSwitcherAndGetOverrideDeviceId`, which is called by `AudioOutputStreamBroker::CreateStream` (follow-up CL: https://chromium-review.googlesource.com/c/chromium/src/+/5685761/8), prevents the browser from caching sink ids for webpages that won't play any sound during their lifetime.
Given that `RenderFrameAudioOutputStreamFactory` is created for every frame regardless, if we decide to store the frame ids during the `RenderFrameAudioOutputStreamFactory` construction, we'll end up storing sink ids for all web pages (including the ones that will not play any sound). Thus, the cache size might grow unnecessarily.
Sunggook, is the performance gain enough to justify this "early improvement"? Like Olga suggested, maybe it could be simpler to just plumb the frame id when the `RenderFrameAudioOutputStreamFactory` is being constructed (https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc;drc=82dff63dbf9db05e9274e11d9128af7b9f51ceaa;bpv=1;bpt=1;l=223)? Also, I suggest only resolving the conversations when both parties agree and there are no more pending questions.
// We don't check device_switcher is valid or not during RemoveSwitcher
// because we just use it as a interger key value for cleanup.Sunggook ChueHmm. Weak pointers will be nulled on the switcher deletion. So it2->get() == device_switcher.get()) is comparing nullptr with nullptr, and potentially deleting some other switcher. This does not add up.
Olga SharonovaThe it2 is collection that is results of the 'main_frame' so it is find to delete any value when 'it2->get() == nullptr and device_switcher.get() is nullptr even if internal it2->get() could be different Switcher from device_switcher.
I will add comments and test to validate it.
Sunggook ChueI'd like to keep the discussion open, since I'm not entirely happy with how it's turning out. but have not figured an alternative proposal yet.
I will file a bug once it lands as the feature flag is disabled. We can't keep this CL open.
Olga Sharonova.
1) It was your choice to skip design discussions and iterate on the code instead. Figuring out a complex design without a clearly communicated high level vision is never easy ant takes time and many back and forth.
2) If you think I have any other agenda besides landing easy-to-maintain code of good quality - you are mistaken. But we cannot land it just for the sake of landing it, because it creates maintenance burden. As you noticed, the review has already uncovered multiple issues, including race conditions.
We need to fix the design issues before it lands.3) The inconsistency of weak pointer treatment hints to a design issue. You can figure it out and fix it. Or I'll think about it when I have a bit of time.
4) The fact that you keep resolving in-progress discussions when I explicitly asked you not to, because it's very hard to keep track of them - I don't even know what to make of it. What's your goal?
I already mentioned my design, please let me know what's your proposal. I don't see any design issue.
Sunggook, maybe we can use a more "traditional" check?
If device_switcher is nullptr, how do we know which device_switcher in the map to erase? Maybe this method should be called only for non-null device_switcher pointers and then we can have another dedicated method to clean-up invalidated pointers. Otherwise, the method will end up doing many more things than just deleting a given frame's device switcher. For example, it could erase other device_switchers besides the one passed as an argument.
Sunggook Chue (Gerrit)
Sunggook Chue added 3 comments![]( "Open in Gerrit")
Thanks Gabriel for chiming in, and again.
We have to access this RenderFrameAudioOutputStreamFactory::Core in the function of PreferredAudioOutputDeviceManager::AddSwitcherAndGetOverrideDeviceId/GetOverrideDeviceId, which is in the IO thread.
1. If we cache in the RenderFrameAudioOutputStreamFactory::Core object, we have to get RenderFrameAudioOutputStreamFactory::Core object through RenderFrameHost object, which is only possible in the UI threads so we can't avoid switch.
2. We might choose cache inside PreferredAudioOutputDeviceManager, which is called from the RenderFrameAudioOutputStreamFactory::Core, it's much better. But the problem is that RenderFrameAudioOutputStreamFactory is called for every RFH. for example, www.bing.com calls RenderFrameAudioOutputStreamFactory ctor 4 times for one tab and imagine user open 10 tabs but no audio play, we end up with 40 calls with cache that lead to worse perf than this ui/io switch cost. that's why I said we need measure perf for perf improvement decision
rather than premature optimization (kind of rule of thumb in perf)
// We don't check device_switcher is valid or not during RemoveSwitcher
// because we just use it as a interger key value for cleanup.Sunggook ChueHmm. Weak pointers will be nulled on the switcher deletion. So it2->get() == device_switcher.get()) is comparing nullptr with nullptr, and potentially deleting some other switcher. This does not add up.
Olga SharonovaThe it2 is collection that is results of the 'main_frame' so it is find to delete any value when 'it2->get() == nullptr and device_switcher.get() is nullptr even if internal it2->get() could be different Switcher from device_switcher.
I will add comments and test to validate it.
Sunggook ChueI'd like to keep the discussion open, since I'm not entirely happy with how it's turning out. but have not figured an alternative proposal yet.
Olga SharonovaI will file a bug once it lands as the feature flag is disabled. We can't keep this CL open.
Olga Sharonova.
Sunggook Chue1) It was your choice to skip design discussions and iterate on the code instead. Figuring out a complex design without a clearly communicated high level vision is never easy ant takes time and many back and forth.
2) If you think I have any other agenda besides landing easy-to-maintain code of good quality - you are mistaken. But we cannot land it just for the sake of landing it, because it creates maintenance burden. As you noticed, the review has already uncovered multiple issues, including race conditions.
We need to fix the design issues before it lands.3) The inconsistency of weak pointer treatment hints to a design issue. You can figure it out and fix it. Or I'll think about it when I have a bit of time.
4) The fact that you keep resolving in-progress discussions when I explicitly asked you not to, because it's very hard to keep track of them - I don't even know what to make of it. What's your goal?
I already mentioned my design, please let me know what's your proposal. I don't see any design issue.
Hmm, I're revisited this and thinking we can improve this by caching globalId, which is called from AddSwitcherAndGetOverrideDeviceId, etc (not from RenderFrameAudioOutputStreamFactory :)).
thanks for suggestion of using GlobalRenderFrameHostId instead of RenderFrameHost.
can we resolve this one?
Related details
- Gabriel Brito
- 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 5 comments![]( "Open in Gerrit")
render_frame_host_id_to_main_frame_id_;Why do we need this mapping? Please document.
RenderFrameAudioOutputStreamFactory is created only when a frame accesses audio info/wants to play audio; it's a per-frame singleton; see RenderFrameHostImpl::CreateAudioOutputStreamFactory().
The size of the cached data is several bytes per frame.
RenderFrameAudioOutputStreamFactory::Core creates output brokers (aka switchers) - so it can pass the id during construction.
RenderFrameAudioOutputStreamFactory::Core::ProviderImpl::Acquire() -> ForwardingAudioStreamFactory::Core::CreateOutputStream().
The issue is not performance, but the design complexity.
In my opinion, this CL is potentially doing early memory optimization by adding unnecessary complexity in the form of extra thread hops and lifetime management complications/weak pointers.
GlobalRenderFrameHostId frame_global_id,Switcher already has it, hasn't it? Why passing it one more time?
// It could not find it when it is called during shudown rightWhat does "during shutdown" mean?
By how much this is better in terms of memory consumption vs storing the main frame id in the switcher?
Related details
- Gabriel Brito
- 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 5 comments![]( "Open in Gerrit")
Why do we need this mapping? Please document.
We want to keep the incoming RFH global id to the its main RFH global id during Add* request so that we can use it during Remove. We don't have to switch to UI thread during RemoveSwitcherEntry.
hmm, when attaching debugger, it is called so many times for every site visit. even vising doc site like w3.org, 4 times are called.
Can you check it again?
Switcher already has it, hasn't it? Why passing it one more time?
correct, thanks.
// It could not find it when it is called during shudown rightWhat does "during shutdown" mean?
process shutdown. I'm not clear this part 100% because RemoveSwitcherEntry is called on IO thread, can the process shutdown during IO threads cleanup? probably not, I will make this one as DCHECK so that we can catch any loophole.
see my comment of RenderFrameAudioOutputStreamFactory creation.
If RenderFrameAudioOutputStreamFactory is created only for AudioOutputStreamBroker, it could increase "8 x 'number of AudioOutputStreamBroker created object'.
but, for design wise, it can scope the feature works on the single place, not multiple classes.
Related details
- Gabriel Brito
- 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. |
Sunggook Chue (Gerrit)
Sunggook Chue added 1 comment![]( "Open in Gerrit")
In real time though, the people will listen to single source at a time, or maximum 2 so "number of AudioOutputStreamBroker" is 1 or 2 even thought they open 20 tabs.
meanwhile, if they open 20 tabs, the number of size by RenderFrameAudioOutputStreamFactory (based on the current observation)it would increase 20 (tabs) x 8 (cache item size) x (number of frames per tab).
Sunggook Chue (Gerrit)
Sunggook Chue added 2 comments![]( "Open in Gerrit")
&GetRenderFrameHostOnUIThread, rfh_id,Olga SharonovaI think I asked it before, but not sure (please don't resolve the comment, otherwise the answer will get lost): have you looked into the option of determining the main host id on the broker creation?
Sunggook ChueSee RenderFrameAudioOutputStreamFactory constructor.
Olga SharonovaIt looks like the object is called from the UI thread object RenderFrameHostImpl, meanwhile AudioOutputSreamBreaker, which call PreferredAudioOutputDeviceManager, is called on IO thread.
Afaik, most of media components (video/audio) that hooks btw Renderer and Utility Services are on IO thread bound.
If needed, we can ask Guido to review this CL (and browser side changes) if needed as he is well aware of it.
Sunggook ChueSunggook, when you reply to the comments, there's a checkbox "resolved". I just asked you in the comment above **to not resolve the discussions in progress**. Please **do not mark the checkbox** for any ongoing discussion or when you are answering a question. I'll mark it as resolved when I've read an answer and have not follow-up questions. Resolved comments are getting lost. This makes my job as a reviewer unnecessary hard. And it causes situations when I have to ask the same question again.
---
If you look at RenderFrameAudioOutputStreamFactory constructor (running on UI thread) you see it's constructing Core. Either RenderFrameAudioOutputStreamFactory
constructor or Core constructor can get the main frame id, since the construction is happening on UI thread. The rest of the Core lives on IO thread and can pass the main frame ID down the line (here https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc;l=323;drc=82dff63dbf9db05e9274e11d9128af7b9f51ceaa;bpv=1;bpt=1, etc); or IO thread code can just access it trough an IO thread getter at some point (to not plumb the same id through too many layers).I do not suggest to rewrite the code immediately, but I'm asking if this has been considered and why the current approach of dynamic fetch of the main frame id was chosen over an option of having a constant main frame id propagated to an IO thread object during construction.
I got your point.
So, we cache global Ids and its parent Ids (if it is sub frame) on IO thread through RenderFrameAudioOutputStreamFactory mechanism and retrieve the main frame global id without switching to UI thread.
The problem is that we need RenderFrameHostImpl object when called on the IO thread with globalId (most Media APIs is called on IO thread from the Renderer) in order to get the RenderFrameAudioOutputStreamFactory object. So switching to UI thread occurs.
Anything I missed?
Olga Sharonova.
1) Could you explain why have you resolved the comment? Especially since it seems like you are asking a question.
2) I mean we cache the main frame global id.
Could you clarify "the problem"? What code are you referring to? (links, methods names?) When I described my idea, I added specific references. Could you do the same?
Olga Sharonova1. We have to access RenderFrameAudioOutputStreamFactory, which is a member of RenderFrameHost, which is accessed from only in the UI thread.
2. RenderFrameAudioOutputStreamFactory is created for every frames but preferred manager is accessed only when audio output request is made. If we cache the values for RenderFrameAudioOutputStreamFactory creation, it would increase the unnecessary cache size.
Sunggook ChueThis is not answering my question though. What code are you referring to? Who "we"? When we have to access RenderFrameAudioOutputStreamFactory? What exactly are you talking about? Which operation?
Olga SharonovaThanks Gabriel for chiming in, and again.
We have to access this RenderFrameAudioOutputStreamFactory::Core in the function of PreferredAudioOutputDeviceManager::AddSwitcherAndGetOverrideDeviceId/GetOverrideDeviceId, which is in the IO thread.
1. If we cache in the RenderFrameAudioOutputStreamFactory::Core object, we have to get RenderFrameAudioOutputStreamFactory::Core object through RenderFrameHost object, which is only possible in the UI threads so we can't avoid switch.
2. We might choose cache inside PreferredAudioOutputDeviceManager, which is called from the RenderFrameAudioOutputStreamFactory::Core, it's much better. But the problem is that RenderFrameAudioOutputStreamFactory is called for every RFH. for example, www.bing.com calls RenderFrameAudioOutputStreamFactory ctor 4 times for one tab and imagine user open 10 tabs but no audio play, we end up with 40 calls with cache that lead to worse perf than this ui/io switch cost. that's why I said we need measure perf for perf improvement decision
rather than premature optimization (kind of rule of thumb in perf)
Sunggook ChueRenderFrameAudioOutputStreamFactory is created only when a frame accesses audio info/wants to play audio; it's a per-frame singleton; see RenderFrameHostImpl::CreateAudioOutputStreamFactory().
The size of the cached data is several bytes per frame.
RenderFrameAudioOutputStreamFactory::Core creates output brokers (aka switchers) - so it can pass the id during construction.
RenderFrameAudioOutputStreamFactory::Core::ProviderImpl::Acquire() -> ForwardingAudioStreamFactory::Core::CreateOutputStream().
The issue is not performance, but the design complexity.In my opinion, this CL is potentially doing early memory optimization by adding unnecessary complexity in the form of extra thread hops and lifetime management complications/weak pointers.
hmm, when attaching debugger, it is called so many times for every site visit. even vising doc site like w3.org, 4 times are called.
Can you check it again?
Here are brief call stacks to call CreateAudioOutputStreamFactory.
content!content::RenderFrameHostImpl::CreateAudioOutputStreamFactory
..
blink_modules!blink::AudioOutputIPCFactory::Impl::RegisterRemoteFactoryOnIOThread
..
blink_modules!blink::AudioOutputIPCFactory::RegisterRemoteFactory
..
content!content::RenderFrameImpl::Initialize
content!content::RenderFrameImpl::CreateFrame
content!content::AgentSchedulingGroup::CreateFrame
Olga Sharonova (Gerrit)
Olga Sharonova added 2 comments![]( "Open in Gerrit")
Hmm, that's unexpected. Thanks for checking!
// We don't check device_switcher is valid or not during RemoveSwitcher
// because we just use it as a interger key value for cleanup.Sunggook ChueHmm. Weak pointers will be nulled on the switcher deletion. So it2->get() == device_switcher.get()) is comparing nullptr with nullptr, and potentially deleting some other switcher. This does not add up.
Olga SharonovaThe it2 is collection that is results of the 'main_frame' so it is find to delete any value when 'it2->get() == nullptr and device_switcher.get() is nullptr even if internal it2->get() could be different Switcher from device_switcher.
I will add comments and test to validate it.
Sunggook ChueI'd like to keep the discussion open, since I'm not entirely happy with how it's turning out. but have not figured an alternative proposal yet.
I will file a bug once it lands as the feature flag is disabled. We can't keep this CL open.
Olga Sharonova.
1) It was your choice to skip design discussions and iterate on the code instead. Figuring out a complex design without a clearly communicated high level vision is never easy ant takes time and many back and forth.
2) If you think I have any other agenda besides landing easy-to-maintain code of good quality - you are mistaken. But we cannot land it just for the sake of landing it, because it creates maintenance burden. As you noticed, the review has already uncovered multiple issues, including race conditions.
We need to fix the design issues before it lands.3) The inconsistency of weak pointer treatment hints to a design issue. You can figure it out and fix it. Or I'll think about it when I have a bit of time.
4) The fact that you keep resolving in-progress discussions when I explicitly asked you not to, because it's very hard to keep track of them - I don't even know what to make of it. What's your goal?
Sunggook ChueI already mentioned my design, please let me know what's your proposal. I don't see any design issue.
Olga SharonovaHmm, I're revisited this and thinking we can improve this by caching globalId, which is called from AddSwitcherAndGetOverrideDeviceId, etc (not from RenderFrameAudioOutputStreamFactory :)).
thanks for suggestion of using GlobalRenderFrameHostId instead of RenderFrameHost.
can we resolve this one?
Sunggook ChueBy how much this is better in terms of memory consumption vs storing the main frame id in the switcher?
Sunggook Chuesee my comment of RenderFrameAudioOutputStreamFactory creation.
If RenderFrameAudioOutputStreamFactory is created only for AudioOutputStreamBroker, it could increase "8 x 'number of AudioOutputStreamBroker created object'.
but, for design wise, it can scope the feature works on the single place, not multiple classes.
In real time though, the people will listen to single source at a time, or maximum 2 so "number of AudioOutputStreamBroker" is 1 or 2 even thought they open 20 tabs.
meanwhile, if they open 20 tabs, the number of size by RenderFrameAudioOutputStreamFactory (based on the current observation)it would increase 20 (tabs) x 8 (cache item size) x (number of frames per tab).
So for some unknown for us reason (maybe a bug), there are 4 RenderFrameAudioOutputStreamFactory::Core objects created per frame.
And you are discussing saving 8 bytes per object, which is 32 bytes per frame, taking into account the current behavior.
Correct?
An idle bing.com tab is over 100 MB.
Do you think the above memory saving is significant enough to influence the design decisions? I don't.
Related details
- Gabriel Brito
- 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 2 comments![]( "Open in Gerrit")
It's not just about memory saving, but its call during RenderFrame initialization, which could be so many as people tend to create many tabs, and its tab has could have many sub frames (RenderFrame). (We have to consider deleting the cache entry during its RenderFrameAudioOutputStreamFactory::Core dtor). We have to access PreferredAudioOutputDeviceManager whenever RenderFrame ctor and dtor that won't be used yet all when no sound is play.
I also think, the benefit of this RenderFrameAudioOutputStreamFactory cache is very small; skip UI/IO switch in the 2 APIs of AddSwitcherAndGetOverrideDeviceId, GetOverrideDeviceId. These APIs are called only when user listen to something.
Related details
- Gabriel Brito
- 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 1 comment![]( "Open in Gerrit")
>We have to consider deleting the cache entry during its RenderFrameAudioOutputStreamFactory::Core dtor
It's where the value will be cached, so I don't understand the concern. Probably you misunderstand what modifications I'm proposing? Could you take another look at the notes and code references I provided in earlier comments?
We have to access PreferredAudioOutputDeviceManager whenever RenderFrame ctor and dtor that won't be used yet all when no sound is play.
What exactly do you mean?
The benefits of caching are:
1) Eliminating most of thread hops => potential lifetime issues (of which I've already found many in earlier versions of this CL) => potential browser crashes. Thus, making the code safer.
2) Making the code much easier to reason about, both in terms of threading and of objects lifetime
3) Likely eliminating the need of weak pointers for AudioOutputDeviceSwitcher
All in all, reduced complexity and probability of bugs, increase maintainability.
As we discussed, memory benefits of the current approach are negligible. And I don't see any other.
And it would take just a couple of hours to sketch the "caching" approach to see if there are any issues with it.
Related details
- Gabriel Brito
- 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 2 comments![]( "Open in Gerrit")
I agree with you that it would be win if we can remove thread switches, but I'm afraid that RenderFrameAudioOutputStreamFactory::Core ctor is navigation path where perf is very important and maybe hard to get approval from the code owner. It would be great if we can find other places where it will be created only when sound plays.
btw, I think, switching thread is kind of common in the MediaDevicesDispatcherHost where it implements mediaDevices namespace APIs.
We can consider the design from each side more.
Related details
- Gabriel Brito
- 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 1 comment![]( "Open in Gerrit")
Exactly which performance concerns are you referring to? Aren't we talking about passing one constant into RenderFrameAudioOutputStreamFactory::Core constructor?
Related details
- Gabriel Brito
- 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 2 comments![]( "Open in Gerrit")
thanks,
My concern is about the page navigation in the RenderFrame where we have many perf bench tests in order to reduce first page navigation speed as users are concern about it. Particularly, the proposal will impact every RenderFrame navigation with not be used scenarios in most cases (if not playing sound)
Related details
- Gabriel Brito
- 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 1 comment![]( "Open in Gerrit")
Impact in which way? What change do you have in mind which would have the impact? Can you estimate the size of it?
(BTW the current approach increases "time to play" by an unbound value, since we hop through the main thread which may be congested)
Related details
- Gabriel Brito
- 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 2 comments![]( "Open in Gerrit")
RenderFrameAudioOutputStreamFactory solutions needs to (1) get the main render frame frame from the requested render frame on the UI thread, and its global id, (2) it adds/delete an entry to the cache that size could be easily over 100.
How about having third reviewer opinion (maybe Guido or who knows about RenderFrame startup) for this?
Related details
- Gabriel Brito
- 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. |
Sunggook Chue (Gerrit)
Sunggook Chue added 1 comment![]( "Open in Gerrit")
meanwhile, I will implement your suggestion and measure its perf, there is a chance that my concern is nothing.
Related details
- Olga Sharonova
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sunggook Chue (Gerrit)
I've run quick test on it, and it looks worse than I thought. vising wsj.com alone create objects (calls chrome!content::RenderFrameAudioOutputStreamFactory::Core::Core) more than 100 times (it also keeps deleting them).
You can easily test this one after attaching debugger on the browser process.
Related details
- Gabriel Brito
- 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. |
Steve Becker (Gerrit)
Steve Becker added 1 comment![]( "Open in Gerrit")
100 is not a large quantity for computers. What about these operations is expensive? Can we measure the amount of time spent?
I think we should avoid premature optimization that adds unneeded complexity.
Related details
- Gabriel Brito
- Olga Sharonova
- 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 1 comment![]( "Open in Gerrit")
I'm measuring the time and couldn't tell the difference in its speed and memory usage (as it deletes what it added, I'm not clear why it does that so many times).
My testing is simply measuring btw JS load and window.onload. We might need perf bench as they are more delicate (probably we have to land for it though).
We can revisit this one at any time, so I can implement what is suggested given no visible perf degradation so far.
Related details
- Gabriel Brito
- 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. |
Sunggook Chue (Gerrit)
There are few uncertainties for the RenderFrameAudioOutputStreamFactory::Core lifetime though, for example,
1. Does every render frame host create one whatever? Not clear.
2. Does deletion ever called even though renderframehost isn't deleted? It looks like, then why?
3. How WebAudio (media element) creation and deletion in JS affect its lifetime?
4. Any chance of missing ID in the PreferredManager so we have to keep both old and new algorithm?
5. Any chance of double registration (Core with same globalId) but deletion once (vice versa)?
probably more,
it would be great if we know these answers clearly. (probably I have to research it more :).)
Olga Sharonova (Gerrit)
Olga Sharonova added 2 comments![]( "Open in Gerrit")
return std::make_unique<AudioOutputAuthorizationHandler>(Please do not forget to address the comments from the previous CL. You can add them here, to keep them in mind.
>RenderFrameAudioOutputStreamFactory solutions needs to (1) get the main render frame frame from the requested render frame on the UI thread, and its global id,
>(2) it adds/delete an entry to the cache that size could be easily over 100.
I'm not sure I understand the concern.
*My proposal is*:
in this place (this is UI thread)
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc;l=194;drc=8e948282d37c0e119e3102236878d6f4d5052c16;bpv=1;bpt=1
to call frame->GetMainFrame()->GetGlobalId() and pass it into the Core constructor.
That's the only thing changing for RenderFrameAudioOutputStreamFactory().
Later on the value can be passed to the broker here
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc;l=120;bpv=1;bpt=1
So each broker knows their main frame, and there is no need to fetch it.
Do you see what I mean?
What delays and performance implications are we discussing?
Re: your questions: unfortunately they are not clear enough. Could you provide more information?
When you invite other reviewers, please keep in mind, you need to ensure they have the full context and an overview of the proposed changes and the alternatives being discussed. Just adding more people as reviewers does not help.
Related details
- Gabriel Brito
- 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. |
Sunggook Chue (Gerrit)
Sunggook Chue added 3 comments![]( "Open in Gerrit")
return std::make_unique<AudioOutputAuthorizationHandler>(Please do not forget to address the comments from the previous CL. You can add them here, to keep them in mind.
I think all are resolved, see https://chromium-review.googlesource.com/c/chromium/src/+/5686893/7?tab=comments.
&GetRenderFrameHostOnUIThread, rfh_id,Olga SharonovaI think I asked it before, but not sure (please don't resolve the comment, otherwise the answer will get lost): have you looked into the option of determining the main host id on the broker creation?
Sunggook ChueSee RenderFrameAudioOutputStreamFactory constructor.
Olga SharonovaIt looks like the object is called from the UI thread object RenderFrameHostImpl, meanwhile AudioOutputSreamBreaker, which call PreferredAudioOutputDeviceManager, is called on IO thread.
Afaik, most of media components (video/audio) that hooks btw Renderer and Utility Services are on IO thread bound.
If needed, we can ask Guido to review this CL (and browser side changes) if needed as he is well aware of it.
Sunggook ChueSunggook, when you reply to the comments, there's a checkbox "resolved". I just asked you in the comment above **to not resolve the discussions in progress**. Please **do not mark the checkbox** for any ongoing discussion or when you are answering a question. I'll mark it as resolved when I've read an answer and have not follow-up questions. Resolved comments are getting lost. This makes my job as a reviewer unnecessary hard. And it causes situations when I have to ask the same question again.
---
If you look at RenderFrameAudioOutputStreamFactory constructor (running on UI thread) you see it's constructing Core. Either RenderFrameAudioOutputStreamFactory
constructor or Core constructor can get the main frame id, since the construction is happening on UI thread. The rest of the Core lives on IO thread and can pass the main frame ID down the line (here https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/media/render_frame_audio_output_stream_factory.cc;l=323;drc=82dff63dbf9db05e9274e11d9128af7b9f51ceaa;bpv=1;bpt=1, etc); or IO thread code can just access it trough an IO thread getter at some point (to not plumb the same id through too many layers).I do not suggest to rewrite the code immediately, but I'm asking if this has been considered and why the current approach of dynamic fetch of the main frame id was chosen over an option of having a constant main frame id propagated to an IO thread object during construction.
I got your point.
So, we cache global Ids and its parent Ids (if it is sub frame) on IO thread through RenderFrameAudioOutputStreamFactory mechanism and retrieve the main frame global id without switching to UI thread.
The problem is that we need RenderFrameHostImpl object when called on the IO thread with globalId (most Media APIs is called on IO thread from the Renderer) in order to get the RenderFrameAudioOutputStreamFactory object. So switching to UI thread occurs.
Anything I missed?
Olga Sharonova.
1) Could you explain why have you resolved the comment? Especially since it seems like you are asking a question.
I will resolve this one as well, reactivate if needed. thanks.
// We don't check device_switcher is valid or not during RemoveSwitcher
// because we just use it as a interger key value for cleanup.initial implementation and tests is positive. updated. thanks.
Related details
- 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 22 comments![]( "Open in Gerrit")
class RenderFrameHostSinkId {
public:
RenderFrameHostSinkId();
~RenderFrameHostSinkId();
RenderFrameHostSinkId(RenderFrameHostSinkId&&);
RenderFrameHostSinkId& operator=(RenderFrameHostSinkId&&);
RenderFrameHostSinkId(const RenderFrameHostSinkId&) = delete;
RenderFrameHostSinkId& operator=(const RenderFrameHostSinkId&) = delete;
void set_preferred_sink_id(const std::string& sink_id) {
preferred_sink_id_ = sink_id;
}
std::string preferred_sink_id() const { return preferred_sink_id_; }
std::vector<AudioOutputDeviceSwitcher*>& active_device_switchers() {
return active_device_switchers_;
}
private:
// The preferred sink id for the top-level page as well as all the sub
// frames.
std::string preferred_sink_id_;
// The active streaming device_switchers that are using the system default
// audio.
std::vector<AudioOutputDeviceSwitcher*> active_device_switchers_;
};
// The map of incoming GlobalRenderFrameHostId to the main
// RenderFrameHostSinkId. We add and delete RenderFrameHostSinkId with the
// main RenderFrameHostSinkId in the
// `RenderFrameAudioOutputStreamFactory::Core::Init()` and deletion of the
// `RenderFrameAudioOutputStreamFactory::Core` object.
std::map<GlobalRenderFrameHostId, GlobalRenderFrameHostId>
render_frame_host_id_to_main_frame_id_;
// The map of GlobalRenderFrameHostId to RenderFrameHostSinkId. It should be
// accessed only in the IO thread.
std::map</*main frame globalId=*/GlobalRenderFrameHostId,
RenderFrameHostSinkId>
render_frame_host_sink_ids_;This looks a bit overly complicated.
Generally, the relationship is
[main_rfh_id] 1-1 [preferred_sink_id]
1
|
*
[rfh_id]
0
|
*
[switchers]
you need to be able to look up
(1) [rfh_id]->[main_rfh_id] for setPreferredSinkId
(2) [main_rfh_id]->switchers for setPreferredSinkId
(3) probably [rfh_id]->switchers for navigation?
right?
anything else?
When do you expect entries to be added/removed, and how many entries do you expect?
class RenderFrameHostSinkId {Please document
void AddRenderFrameHostid(GlobalRenderFrameHostId rfh_id,
GlobalRenderFrameHostId main_frame_global_id);
void DeleteRenderFrameHostid(GlobalRenderFrameHostId rfh_id);Please document
HostId
Also use the same naming scheme, for example rfh_id/main_rfh_id
void RemoveRenderFrameHostEntry(RenderFrameHost* render_frame_host);Which entry? You have two maps.
Please document what it does and name it accordingly. "UnregisterSwitchersFoFrame" or something.
void RemoveSwitcherEntry(GlobalRenderFrameHostId rfh_id,RemoveSwitcher
// It should be called on IO thread only.You can add one overarching comment that all methods except specifically mentioned are called on IO thread.
std::string GetOverrideDeviceId(GlobalRenderFrameHostId rfh_id);GetPreferrdSinkId (name in pairs: set/get)
// It will overrided device id if found, or empty string.What does it mean?
// `process_id` is the id of the process. It is used for retrieving the
// render frame host object.What's the purpose of this sentense?
// Get the override device id for the render frame host if an entry exists.And if it does not?
void RegisterDeviceOutputSwitcher(GlobalRenderFrameHostId rfh_id,Please name them in a pair: Just AddSwitcher()/RemoveSwitcher()
// Add the device switcher to the active device switchers list and get the
// override device id for the render frame host if an entry exists.
// `device_switcher` is the device switcher to be added to the active device.
// switchers list.
// It returnsplease fix
GlobalRenderFrameHostId rfh_id);Should be the first parameter
SetPreferredSinkIdCallback callback,should be the last parameter
virtual void SetPreferredSinkId(const std::string& sink_id,Please document what it actually does (switches the default sink for all subframes of some frame something something)
// from the WebContentsObserver. Synchronization locks are not used for
// accessing the cache; instead, it is accessed exclusively on the IO thread,
// So if an API is called on the IO thread with a render frame host ID, the
// request is forwarded to the UI thread to get the RenderFrameHost object
// and then returns to the IO thread to manage the cache.Not relevant any longer?
Also, please do not document implementation in the header.
// The class is created and destroyed on the IO thread during
// MediaStreamManager initialization and destruction. While most APIs arePlease do not add implicit assumptions not guaranteed by the class.
// frame as well as all the sub pages if they use system default audio output.
// It caches the preferred sink id with each top RenderFrameHost object and
// forwards the request to the corresponding audio output stream in the
// audio service if streaming is in active. The cached sink id also will be
// substituted for the new audio output stream if the requested stream is
// default. The cache's key is RenderFrameHost object, which can be retrieved
// only in the UI thread from render frame host id.Please move implementation details description into .cc. Please discuss the API from the client perspective in the header.
return std::make_unique<AudioOutputAuthorizationHandler>(Sunggook ChuePlease do not forget to address the comments from the previous CL. You can add them here, to keep them in mind.
I think all are resolved, see https://chromium-review.googlesource.com/c/chromium/src/+/5686893/7?tab=comments.
I only see it marked as resolved https://chromium-review.googlesource.com/c/chromium/src/+/5686893/comment/a78938d3_bad42416/
base::BindOnce(&CreateAudioOutputAuthHandlerOnUIThread, rfh_id),There is no need to create the handler every time, please reconsider.
See RenderFrameAudioOutputStreamFactory::Core
std::string PreferredAudioOutputDeviceManager::GetOverrideDeviceIdWithMainFrame(Instead of adding a second layer of "WithMainFrame" methods, it's much more readable to add getMainRenderFrameHostId(GlobalRenderFrameHostId rfh_id)
// We don't check device_switcher is valid or not during RemoveSwitcher
// because we just use it as a interger key value for cleanup.Since you modified the proposal above - please provide a high-level description and considerations, so the reviewers don't have to read between the lines.
Related details
- Gabriel Brito
- 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. |
Olga Sharonova (Gerrit)
Olga Sharonova added 1 comment![]( "Open in Gerrit")
// `RenderFrameAudioOutputStreamFactory::Core::Init()` and deletion of theIs the code there?
(Also please do not refer to the external context)
Sunggook Chue (Gerrit)
Sunggook Chue added 23 comments![]( "Open in Gerrit")
class RenderFrameHostSinkId {
public:
RenderFrameHostSinkId();
~RenderFrameHostSinkId();
RenderFrameHostSinkId(RenderFrameHostSinkId&&);
RenderFrameHostSinkId& operator=(RenderFrameHostSinkId&&);
RenderFrameHostSinkId(const RenderFrameHostSinkId&) = delete;
RenderFrameHostSinkId& operator=(const RenderFrameHostSinkId&) = delete;
void set_preferred_sink_id(const std::string& sink_id) {
preferred_sink_id_ = sink_id;
}
std::string preferred_sink_id() const { return preferred_sink_id_; }
std::vector<AudioOutputDeviceSwitcher*>& active_device_switchers() {
return active_device_switchers_;
}
private:
// The preferred sink id for the top-level page as well as all the sub
// frames.
std::string preferred_sink_id_;
// The active streaming device_switchers that are using the system default
// audio.
std::vector<AudioOutputDeviceSwitcher*> active_device_switchers_;
};
// The map of incoming GlobalRenderFrameHostId to the main
// RenderFrameHostSinkId. We add and delete RenderFrameHostSinkId with the
// main RenderFrameHostSinkId in the
// `RenderFrameAudioOutputStreamFactory::Core::Init()` and deletion of the
// `RenderFrameAudioOutputStreamFactory::Core` object.
std::map<GlobalRenderFrameHostId, GlobalRenderFrameHostId>
render_frame_host_id_to_main_frame_id_;
// The map of GlobalRenderFrameHostId to RenderFrameHostSinkId. It should be
// accessed only in the IO thread.
std::map</*main frame globalId=*/GlobalRenderFrameHostId,
RenderFrameHostSinkId>
render_frame_host_sink_ids_;This looks a bit overly complicated.
Generally, the relationship is
[main_rfh_id] 1-1 [preferred_sink_id]
1
|
*
[rfh_id]
0
|
*
[switchers]you need to be able to look up
(1) [rfh_id]->[main_rfh_id] for setPreferredSinkId
(2) [main_rfh_id]->switchers for setPreferredSinkId
(3) probably [rfh_id]->switchers for navigation?right?
anything else?When do you expect entries to be added/removed, and how many entries do you expect?
yes, it is right. I don't see anything else.
for render_frame_host_sink_ids, the max entries will be same with the number of playing Switchers that are AudioOutputStreamBroker in the browser. people could playing sound from all of their frames but they listen 1 or 2 at that time. the paused play will terminate the AudioOutputStreamBroker after certain threshold. So the maximum number won't be much, less than 3. I guess.
for render_frame_host_id_to_main_frame_id_, it is more interesting one. In theory (you know this one more than I do) it is as many as RenderFrameAudioInputStreamFactory::Core object, which could be per frame. When quick testing though, it is quite smaller than number of frames even though it is created per frame initially but deleted somehow later. In the end, the frame that creates RenderFrameAudioInputStreamFactory::Core looks have RenderFrameAudioInputStreamFactory::Core objects.
so I expects this one is also same with render_frame_host_sink_ids.
Please reactivate this one if you have requests as it is about the question and I answered so I resolve it (few unresolved help focus on discussion).
// `RenderFrameAudioOutputStreamFactory::Core::Init()` and deletion of theIs the code there?
(Also please do not refer to the external context)
but you know it is correct if you see the next CL? I thought the benefit of these relation CL is to know the future work so we can have complete class that doesn't have to updated the next CL.
anyway, I will remove this one.
class RenderFrameHostSinkId {Sunggook ChuePlease document
Done
void AddRenderFrameHostid(GlobalRenderFrameHostId rfh_id,
GlobalRenderFrameHostId main_frame_global_id);
void DeleteRenderFrameHostid(GlobalRenderFrameHostId rfh_id);Please document
HostId
Also use the same naming scheme, for example rfh_id/main_rfh_id
Done
void RemoveRenderFrameHostEntry(RenderFrameHost* render_frame_host);Which entry? You have two maps.
Please document what it does and name it accordingly. "UnregisterSwitchersFoFrame" or something.
Done
void RemoveSwitcherEntry(GlobalRenderFrameHostId rfh_id,Sunggook ChueRemoveSwitcher
using RemoveDeviceOutputSwitcher.
You can add one overarching comment that all methods except specifically mentioned are called on IO thread.
thanks,
std::string GetOverrideDeviceId(GlobalRenderFrameHostId rfh_id);GetPreferrdSinkId (name in pairs: set/get)
good name.
// It will overrided device id if found, or empty string.Sunggook ChueWhat does it mean?
removed as it is confusing.
// `process_id` is the id of the process. It is used for retrieving the
// render frame host object.What's the purpose of this sentense?
no needed anymore.
// Get the override device id for the render frame host if an entry exists.And if it does not?
Done
void RegisterDeviceOutputSwitcher(GlobalRenderFrameHostId rfh_id,Please name them in a pair: Just AddSwitcher()/RemoveSwitcher()
Done
// Add the device switcher to the active device switchers list and get the
// override device id for the render frame host if an entry exists.
// `device_switcher` is the device switcher to be added to the active device.
// switchers list.
// It returnsSunggook Chueplease fix
Done
Should be the first parameter
Done
should be the last parameter
Done
virtual void SetPreferredSinkId(const std::string& sink_id,Please document what it actually does (switches the default sink for all subframes of some frame something something)
Done
// from the WebContentsObserver. Synchronization locks are not used for
// accessing the cache; instead, it is accessed exclusively on the IO thread,
// So if an API is called on the IO thread with a render frame host ID, the
// request is forwarded to the UI thread to get the RenderFrameHost object
// and then returns to the IO thread to manage the cache.Not relevant any longer?
Also, please do not document implementation in the header.
Done
// The class is created and destroyed on the IO thread during
// MediaStreamManager initialization and destruction. While most APIs arePlease do not add implicit assumptions not guaranteed by the class.
replaced "The class is expected to be created and destroyed on the IO thread as
// as singleton per the browser instance"
// frame as well as all the sub pages if they use system default audio output.
// It caches the preferred sink id with each top RenderFrameHost object and
// forwards the request to the corresponding audio output stream in the
// audio service if streaming is in active. The cached sink id also will be
// substituted for the new audio output stream if the requested stream is
// default. The cache's key is RenderFrameHost object, which can be retrieved
// only in the UI thread from render frame host id.Please move implementation details description into .cc. Please discuss the API from the client perspective in the header.
Move to the RegisterDeviceOutputSwitcher API definition.
return std::make_unique<AudioOutputAuthorizationHandler>(Sunggook ChuePlease do not forget to address the comments from the previous CL. You can add them here, to keep them in mind.
Olga SharonovaI think all are resolved, see https://chromium-review.googlesource.com/c/chromium/src/+/5686893/7?tab=comments.
I only see it marked as resolved https://chromium-review.googlesource.com/c/chromium/src/+/5686893/comment/a78938d3_bad42416/
Did you suggest to register process id from the ::Core like main RHF ID?
I think setPreferredSinkId can be called even when ::Core object isn't called as ::Core object is created only when audio play is requested?
base::BindOnce(&CreateAudioOutputAuthHandlerOnUIThread, rfh_id),There is no need to create the handler every time, please reconsider.
See RenderFrameAudioOutputStreamFactory::Core
yes, you're right, we can have per process authorization_handler_, hmm, do you think have to have another table for "process - authorization_handler",
Isn't it clean design to create authorization_handler for every request as (1) we don't expect many setPreferredSinkId calls for the main page lifetime (assuming main page run on its own render process), (2) no need to listen to process termination to clean it (not sure if chromium has such event)?
std::string PreferredAudioOutputDeviceManager::GetOverrideDeviceIdWithMainFrame(Instead of adding a second layer of "WithMainFrame" methods, it's much more readable to add getMainRenderFrameHostId(GlobalRenderFrameHostId rfh_id)
Done
updated description, if needed, you can update it if it helps. thanks.
Related details
- Gabriel Brito
- 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. |
Sunggook Chue (Gerrit)
Sunggook Chue added 2 comments![]( "Open in Gerrit")
Hi Olga, Please let me know if you have any more comments on this CL, thanks.
// We don't check device_switcher is valid or not during RemoveSwitcher
// because we just use it as a interger key value for cleanup.- 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 8 comments![]( "Open in Gerrit")
1) Please fix the CL chain, and keep it up to date for all next review rounds.
2) Please restore the design doc and close it for public editing.
Sunggook ChueThis looks a bit overly complicated.
Generally, the relationship is
[main_rfh_id] 1-1 [preferred_sink_id]
1
|
*
[rfh_id]
0
|
*
[switchers]you need to be able to look up
(1) [rfh_id]->[main_rfh_id] for setPreferredSinkId
(2) [main_rfh_id]->switchers for setPreferredSinkId
(3) probably [rfh_id]->switchers for navigation?right?
anything else?When do you expect entries to be added/removed, and how many entries do you expect?
yes, it is right. I don't see anything else.
for render_frame_host_sink_ids, the max entries will be same with the number of playing Switchers that are AudioOutputStreamBroker in the browser. people could playing sound from all of their frames but they listen 1 or 2 at that time. the paused play will terminate the AudioOutputStreamBroker after certain threshold. So the maximum number won't be much, less than 3. I guess.
for render_frame_host_id_to_main_frame_id_, it is more interesting one. In theory (you know this one more than I do) it is as many as RenderFrameAudioInputStreamFactory::Core object, which could be per frame. When quick testing though, it is quite smaller than number of frames even though it is created per frame initially but deleted somehow later. In the end, the frame that creates RenderFrameAudioInputStreamFactory::Core looks have RenderFrameAudioInputStreamFactory::Core objects.
so I expects this one is also same with render_frame_host_sink_ids.
Please reactivate this one if you have requests as it is about the question and I answered so I resolve it (few unresolved help focus on discussion).
Please don't resolve when you answer questions, I already explained why: exactly to keep the focus on the discussion The reviewers will resolve if they are happy with the answer and have no follow-up questions.
Since you expect only a handful of objects, there is no need to use maps, a search in a vector is just as fast.
Let's say you have a vector (or a map if you like) of {main_frame_id, preferred_sink_id}
and a vector of
{Switcher, frame_id, main_frame_id}
Wouldn't it make the code easier to reason about? (GlobalRenderFrameHostId to GlobalRenderFrameHostId is a bit too hard on the eye)
Also, in my initial proposal I meant for Switcher to store both main frame id and frame id inside itself - they would make the code and the API even simpler
// `RenderFrameAudioOutputStreamFactory::Core::Init()` and deletion of theSunggook ChueIs the code there?
(Also please do not refer to the external context)
but you know it is correct if you see the next CL? I thought the benefit of these relation CL is to know the future work so we can have complete class that doesn't have to updated the next CL.
anyway, I will remove this one.
It was not in the next CL. and the CL chain is broken.
return std::make_unique<AudioOutputAuthorizationHandler>(Sunggook ChuePlease do not forget to address the comments from the previous CL. You can add them here, to keep them in mind.
Olga SharonovaI think all are resolved, see https://chromium-review.googlesource.com/c/chromium/src/+/5686893/7?tab=comments.
Sunggook ChueI only see it marked as resolved https://chromium-review.googlesource.com/c/chromium/src/+/5686893/comment/a78938d3_bad42416/
Did you suggest to register process id from the ::Core like main RHF ID?
I think setPreferredSinkId can be called even when ::Core object isn't called as ::Core object is created only when audio play is requested?
In this specific comment we were discussing on which thread AudioOutputAuthorizationHandler is created.
Could you clarify the question you are asking, and ask it in the relevant place of the CL?
base::BindOnce(&CreateAudioOutputAuthHandlerOnUIThread, rfh_id),Sunggook ChueThere is no need to create the handler every time, please reconsider.
See RenderFrameAudioOutputStreamFactory::Core
yes, you're right, we can have per process authorization_handler_, hmm, do you think have to have another table for "process - authorization_handler",
Isn't it clean design to create authorization_handler for every request as (1) we don't expect many setPreferredSinkId calls for the main page lifetime (assuming main page run on its own render process), (2) no need to listen to process termination to clean it (not sure if chromium has such event)?
Don't you listen to the process termination already?
std::string PreferredAudioOutputDeviceManager::GetPreferredSinkId(const std::string& (no need to make early copies)
void PreferredAudioOutputDeviceManager::UnregisterSwitchersFoFrame(Just UnregisterMainFrame?
In any case, please indicate in the name that's it's only the main frame
GlobalRenderFrameHostIdconst GlobalRenderFrameHostId&
Related details
- 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 8 comments![]( "Open in Gerrit")
1) Please fix the CL chain, and keep it up to date for all next review rounds.
2) Please restore the design doc and close it for public editing.
The latest CL are 4 and it is up to date, I abandoned not used ones. please let me know if there is anything else I need to do.
I repaired the doc, I restricted the commentor with invite based from link.
for map vs vector, I said that the end result is handful, but not in the middle of it. what I've observed that RenderFrameAudioInputStreamFactory::Core can be created 100 times, but deleted 100 times as well. so at some point, it could be large numbers so using map seems better.
map<GlobalRenderFrameHostId to GlobalRenderFrameHostId> is called from the RenderFrameAudioInputStreamFactory::Core when no Switcher is available. Switcher will read access of it after the entry was inserted by Core.
I might change the internal class name of RenderFrameHostSinkId as 'PreferredSinkIdToSwitcher' because RenderFrameHostSinkId is confusing with a name of GlobalRenderFrameHostId. please suggest if you have better name.
// `RenderFrameAudioOutputStreamFactory::Core::Init()` and deletion of theSunggook ChueIs the code there?
(Also please do not refer to the external context)
Olga Sharonovabut you know it is correct if you see the next CL? I thought the benefit of these relation CL is to know the future work so we can have complete class that doesn't have to updated the next CL.
anyway, I will remove this one.
It was not in the next CL. and the CL chain is broken.
the next CL is https://chromium-review.googlesource.com/c/chromium/src/+/5814796/6. Please ignore any gray color (abandoned), somehow gerrit does not remove it automatically, maybe I have to unset and upload for the abandoned, I will try it.
return std::make_unique<AudioOutputAuthorizationHandler>(Sunggook ChuePlease do not forget to address the comments from the previous CL. You can add them here, to keep them in mind.
Olga SharonovaI think all are resolved, see https://chromium-review.googlesource.com/c/chromium/src/+/5686893/7?tab=comments.
Sunggook ChueI only see it marked as resolved https://chromium-review.googlesource.com/c/chromium/src/+/5686893/comment/a78938d3_bad42416/
Olga SharonovaDid you suggest to register process id from the ::Core like main RHF ID?
I think setPreferredSinkId can be called even when ::Core object isn't called as ::Core object is created only when audio play is requested?
In this specific comment we were discussing on which thread AudioOutputAuthorizationHandler is created.
Could you clarify the question you are asking, and ask it in the relevant place of the CL?
It looks you ask static render process ID for AudioOutputAuthorizationHandler; do not create AudioOutputAuthorizationHandler everytime.
we can discuss it on the below comment.
base::BindOnce(&CreateAudioOutputAuthHandlerOnUIThread, rfh_id),Sunggook ChueThere is no need to create the handler every time, please reconsider.
See RenderFrameAudioOutputStreamFactory::Core
Olga Sharonovayes, you're right, we can have per process authorization_handler_, hmm, do you think have to have another table for "process - authorization_handler",
Isn't it clean design to create authorization_handler for every request as (1) we don't expect many setPreferredSinkId calls for the main page lifetime (assuming main page run on its own render process), (2) no need to listen to process termination to clean it (not sure if chromium has such event)?
Don't you listen to the process termination already?
no, we don't listen to, just Render frame host termination. multiple render frame hosts can be in the single system process, or even sub frame can have its own system process.
We might adjust the code little bit such that we check the sub or main frames before creating AudioOutputAuthorizationHandler, and create/cache for main frame RFH with preferred sink id.
I will update it, please let me know if you have better idea.
std::string PreferredAudioOutputDeviceManager::GetPreferredSinkId(const std::string& (no need to make early copies)
it is a return value, and returning
GlobalRenderFrameHostIdSunggook Chueconst GlobalRenderFrameHostId&
Done
Related details
- 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 6 comments![]( "Open in Gerrit")
Sunggook Chue1) Please fix the CL chain, and keep it up to date for all next review rounds.
2) Please restore the design doc and close it for public editing.
The latest CL are 4 and it is up to date, I abandoned not used ones. please let me know if there is anything else I need to do.
I repaired the doc, I restricted the commentor with invite based from link.
Thanks.
The CL chain needs to be rebased, now all follow-up CLs are "indirect relation"
How about Switcher storing its main and native frame ids?
I'd like the code to be a bit more readable.
// `RenderFrameAudioOutputStreamFactory::Core::Init()` and deletion of theSunggook ChueIs the code there?
(Also please do not refer to the external context)
Olga Sharonovabut you know it is correct if you see the next CL? I thought the benefit of these relation CL is to know the future work so we can have complete class that doesn't have to updated the next CL.
anyway, I will remove this one.
Sunggook ChueIt was not in the next CL. and the CL chain is broken.
the next CL is https://chromium-review.googlesource.com/c/chromium/src/+/5814796/6. Please ignore any gray color (abandoned), somehow gerrit does not remove it automatically, maybe I have to unset and upload for the abandoned, I will try it.
Have you tried
return std::make_unique<AudioOutputAuthorizationHandler>(Sunggook ChuePlease do not forget to address the comments from the previous CL. You can add them here, to keep them in mind.
Olga SharonovaI think all are resolved, see https://chromium-review.googlesource.com/c/chromium/src/+/5686893/7?tab=comments.
Sunggook ChueI only see it marked as resolved https://chromium-review.googlesource.com/c/chromium/src/+/5686893/comment/a78938d3_bad42416/
Olga SharonovaDid you suggest to register process id from the ::Core like main RHF ID?
I think setPreferredSinkId can be called even when ::Core object isn't called as ::Core object is created only when audio play is requested?
Sunggook ChueIn this specific comment we were discussing on which thread AudioOutputAuthorizationHandler is created.
Could you clarify the question you are asking, and ask it in the relevant place of the CL?
It looks you ask static render process ID for AudioOutputAuthorizationHandler; do not create AudioOutputAuthorizationHandler everytime.
we can discuss it on the below comment.
Could you please take a look at the original CL, where we are discussing that creating AudioOutputAuthorizationHandler on UI thread does not look reasonable, since AudioOutputAuthorizationHandler documentation sats explicitly that this is an IO thread class.
base::BindOnce(&CreateAudioOutputAuthHandlerOnUIThread, rfh_id),Sunggook ChueThere is no need to create the handler every time, please reconsider.
See RenderFrameAudioOutputStreamFactory::Core
Olga Sharonovayes, you're right, we can have per process authorization_handler_, hmm, do you think have to have another table for "process - authorization_handler",
Isn't it clean design to create authorization_handler for every request as (1) we don't expect many setPreferredSinkId calls for the main page lifetime (assuming main page run on its own render process), (2) no need to listen to process termination to clean it (not sure if chromium has such event)?
Sunggook ChueDon't you listen to the process termination already?
no, we don't listen to, just Render frame host termination. multiple render frame hosts can be in the single system process, or even sub frame can have its own system process.
We might adjust the code little bit such that we check the sub or main frames before creating AudioOutputAuthorizationHandler, and create/cache for main frame RFH with preferred sink id.
I will update it, please let me know if you have better idea.
Ok, let's put caching aside for now.
std::string PreferredAudioOutputDeviceManager::GetPreferredSinkId(Sunggook Chueconst std::string& (no need to make early copies)
it is a return value, and returning
- 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")
Single frame id could have multiple DeviceSwitchers, so frame id doesn't 1:1 match with frame id, instead providing a method to DeviceSwitcher for retrieving frame ids seems better idea only when it is needed.
// `RenderFrameAudioOutputStreamFactory::Core::Init()` and deletion of theSunggook ChueIs the code there?
(Also please do not refer to the external context)
Olga Sharonovabut you know it is correct if you see the next CL? I thought the benefit of these relation CL is to know the future work so we can have complete class that doesn't have to updated the next CL.
anyway, I will remove this one.
Sunggook ChueIt was not in the next CL. and the CL chain is broken.
Olga Sharonovathe next CL is https://chromium-review.googlesource.com/c/chromium/src/+/5814796/6. Please ignore any gray color (abandoned), somehow gerrit does not remove it automatically, maybe I have to unset and upload for the abandoned, I will try it.
Have you tried
I tried, git cl upload command does not work yet all as it said it is abandoned. It is my first time of using relation CLs, many mistakes, sorry about that.
return std::make_unique<AudioOutputAuthorizationHandler>(Sunggook ChuePlease do not forget to address the comments from the previous CL. You can add them here, to keep them in mind.
Olga SharonovaI think all are resolved, see https://chromium-review.googlesource.com/c/chromium/src/+/5686893/7?tab=comments.
Sunggook ChueI only see it marked as resolved https://chromium-review.googlesource.com/c/chromium/src/+/5686893/comment/a78938d3_bad42416/
Olga SharonovaDid you suggest to register process id from the ::Core like main RHF ID?
I think setPreferredSinkId can be called even when ::Core object isn't called as ::Core object is created only when audio play is requested?
Sunggook ChueIn this specific comment we were discussing on which thread AudioOutputAuthorizationHandler is created.
Could you clarify the question you are asking, and ask it in the relevant place of the CL?
Olga SharonovaIt looks you ask static render process ID for AudioOutputAuthorizationHandler; do not create AudioOutputAuthorizationHandler everytime.
we can discuss it on the below comment.
Could you please take a look at the original CL, where we are discussing that creating AudioOutputAuthorizationHandler on UI thread does not look reasonable, since AudioOutputAuthorizationHandler documentation sats explicitly that this is an IO thread class.
It isn't just AudioOutputAuthorizationHandler anymore.
We also check if the given RFH id is main or not for setPreferredSinkId when there is Core object ever called before it. The only way is to check is calling RenderFrameHost::GetMainFrame, which is doable (getting RenderFrameHost object from the ID) only in the UI thread.
The latest commit has really minimized the UI thread use; just once per the unique RFH, which takes place only in the first setPreferredSinkId request.
base::BindOnce(&CreateAudioOutputAuthHandlerOnUIThread, rfh_id),Sunggook ChueThere is no need to create the handler every time, please reconsider.
See RenderFrameAudioOutputStreamFactory::Core
Olga Sharonovayes, you're right, we can have per process authorization_handler_, hmm, do you think have to have another table for "process - authorization_handler",
Isn't it clean design to create authorization_handler for every request as (1) we don't expect many setPreferredSinkId calls for the main page lifetime (assuming main page run on its own render process), (2) no need to listen to process termination to clean it (not sure if chromium has such event)?
Sunggook ChueDon't you listen to the process termination already?
Olga Sharonovano, we don't listen to, just Render frame host termination. multiple render frame hosts can be in the single system process, or even sub frame can have its own system process.
We might adjust the code little bit such that we check the sub or main frames before creating AudioOutputAuthorizationHandler, and create/cache for main frame RFH with preferred sink id.
I will update it, please let me know if you have better idea.
Ok, let's put caching aside for now.
see the latest code, I cached it for render frame host as suggested, but not by process id. it works well. thanks.
std::string PreferredAudioOutputDeviceManager::GetPreferredSinkId(Sunggook Chueconst std::string& (no need to make early copies)
Olga Sharonovait is a return value, and returning
?
updated as suggested,
void PreferredAudioOutputDeviceManager::UnregisterSwitchersFoFrame(Just UnregisterMainFrame?
In any case, please indicate in the name that's it's only the main frame
- 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 7 comments![]( "Open in Gerrit")
virtual void RequestDeviceAuthorization(Ditto.
virtual ~AudioOutputAuthorizationHandler();If making virtual for tests, please note it in the comment.
std::unique_ptr<AudioOutputAuthorizationHandler> authorization_handler_;I think a frame has to use its own authorization handler, because a cross-origin frame does not necessarily have the same permissions as the main one.
+Guido.
Could you clarify? I did not understand the statement.
Also, could you provide the reasoning?
My reasoning is external mapping needs maintenance and thus is error-prone. I don't see any particular issues with mapping not being 1:1.
// AudioOutputAuthorizationHandler is created per main RenderFrameHost object,Haven't we agreed to not cache it a while back?
return std::make_unique<AudioOutputAuthorizationHandler>(Sunggook ChuePlease do not forget to address the comments from the previous CL. You can add them here, to keep them in mind.
Olga SharonovaI think all are resolved, see https://chromium-review.googlesource.com/c/chromium/src/+/5686893/7?tab=comments.
Sunggook ChueI only see it marked as resolved https://chromium-review.googlesource.com/c/chromium/src/+/5686893/comment/a78938d3_bad42416/
Olga SharonovaDid you suggest to register process id from the ::Core like main RHF ID?
I think setPreferredSinkId can be called even when ::Core object isn't called as ::Core object is created only when audio play is requested?
Sunggook ChueIn this specific comment we were discussing on which thread AudioOutputAuthorizationHandler is created.
Could you clarify the question you are asking, and ask it in the relevant place of the CL?
Olga SharonovaIt looks you ask static render process ID for AudioOutputAuthorizationHandler; do not create AudioOutputAuthorizationHandler everytime.
we can discuss it on the below comment.
Sunggook ChueCould you please take a look at the original CL, where we are discussing that creating AudioOutputAuthorizationHandler on UI thread does not look reasonable, since AudioOutputAuthorizationHandler documentation sats explicitly that this is an IO thread class.
It isn't just AudioOutputAuthorizationHandler anymore.
We also check if the given RFH id is main or not for setPreferredSinkId when there is Core object ever called before it. The only way is to check is calling RenderFrameHost::GetMainFrame, which is doable (getting RenderFrameHost object from the ID) only in the UI thread.
The latest commit has really minimized the UI thread use; just once per the unique RFH, which takes place only in the first setPreferredSinkId request.
Please see my previous comments: what I'm asking is to move authorization handler creation to IO thread. There is nothing preventing it, no matter what needs to be done on UI thread prior to that.
Related details
- Guido Urdaneta
- 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")
media::AudioSystem* audio_system =
BrowserMainLoop::GetInstance()->audio_system();
MediaStreamManager* media_stream_manager =
BrowserMainLoop::GetInstance()->media_stream_manager();These can be passed into PreferredAudioOutputDeviceManager constructor, no need to access BrowserMainLoop.
Sunggook Chue (Gerrit)
Sunggook Chue added 2 comments![]( "Open in Gerrit")
std::unique_ptr<AudioOutputAuthorizationHandler> authorization_handler_;I think a frame has to use its own authorization handler, because a cross-origin frame does not necessarily have the same permissions as the main one.
+Guido.
Authorization handler is used by the main frame alone, not from the sub frames. The code returns an error if it is a sub frame before calling auth handler.
Do we still have an issue here?
// AudioOutputAuthorizationHandler is created per main RenderFrameHost object,Haven't we agreed to not cache it a while back?
yes, we did, but I realized that we could cache per main RenderFrameHost that will help less creation of auth handler.
Please let me know if there is an issue here?
Related details
- Guido Urdaneta
- Olga Sharonova
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")
Sunggook Chue1) Please fix the CL chain, and keep it up to date for all next review rounds.
2) Please restore the design doc and close it for public editing.
Olga SharonovaThe latest CL are 4 and it is up to date, I abandoned not used ones. please let me know if there is anything else I need to do.
I repaired the doc, I restricted the commentor with invite based from link.
Thanks.
The CL chain needs to be rebased, now all follow-up CLs are "indirect relation"
I created the next branch with -b command, and --set-upstream-to=localBranch. it works fine in terms of 'fetch' command.
Based on research, i tried to convert indirect to direct by resetting --set-upstream-to=origin/localBranch instead of --set-upstream-to=localBranch, it failed due to missing origin/localBranch. will researching it.
virtual void RequestDeviceAuthorization(Sunggook ChueDitto.
Done
If making virtual for tests, please note it in the comment.
Done
std::unique_ptr<AudioOutputAuthorizationHandler> authorization_handler_;Sunggook ChueI think a frame has to use its own authorization handler, because a cross-origin frame does not necessarily have the same permissions as the main one.
+Guido.
Authorization handler is used by the main frame alone, not from the sub frames. The code returns an error if it is a sub frame before calling auth handler.
Do we still have an issue here?
I think it again, we can avoid this cross-origin issue yet all by creating AudioOutputAuthorizationHandler per request as it is on IO thread now, so much effective.
I'm fine to have it, see how it looks like.
// AudioOutputAuthorizationHandler is created per main RenderFrameHost object,Sunggook ChueHaven't we agreed to not cache it a while back?
yes, we did, but I realized that we could cache per main RenderFrameHost that will help less creation of auth handler.
Please let me know if there is an issue here?
ok, I will remove the cache for allowing cross-origin sub frames can call it.
media::AudioSystem* audio_system =
BrowserMainLoop::GetInstance()->audio_system();
MediaStreamManager* media_stream_manager =
BrowserMainLoop::GetInstance()->media_stream_manager();These can be passed into PreferredAudioOutputDeviceManager constructor, no need to access BrowserMainLoop.
it's good idea, PreferredAudioOutputDeviceManager relies on the MediaStreamManager and created from it. I will update it. thanks.
// Sub frame can't call the API.Sunggook ChueWny?
Initially, I proposed no allowance from the sub frames and but realized that we allow it later. sorry for confusion.
return std::make_unique<AudioOutputAuthorizationHandler>(Sunggook ChuePlease do not forget to address the comments from the previous CL. You can add them here, to keep them in mind.
Olga SharonovaI think all are resolved, see https://chromium-review.googlesource.com/c/chromium/src/+/5686893/7?tab=comments.
Sunggook ChueI only see it marked as resolved https://chromium-review.googlesource.com/c/chromium/src/+/5686893/comment/a78938d3_bad42416/
Olga SharonovaDid you suggest to register process id from the ::Core like main RHF ID?
I think setPreferredSinkId can be called even when ::Core object isn't called as ::Core object is created only when audio play is requested?
Sunggook ChueIn this specific comment we were discussing on which thread AudioOutputAuthorizationHandler is created.
Could you clarify the question you are asking, and ask it in the relevant place of the CL?
Olga SharonovaIt looks you ask static render process ID for AudioOutputAuthorizationHandler; do not create AudioOutputAuthorizationHandler everytime.
we can discuss it on the below comment.
Sunggook ChueCould you please take a look at the original CL, where we are discussing that creating AudioOutputAuthorizationHandler on UI thread does not look reasonable, since AudioOutputAuthorizationHandler documentation sats explicitly that this is an IO thread class.
Olga SharonovaIt isn't just AudioOutputAuthorizationHandler anymore.
We also check if the given RFH id is main or not for setPreferredSinkId when there is Core object ever called before it. The only way is to check is calling RenderFrameHost::GetMainFrame, which is doable (getting RenderFrameHost object from the ID) only in the UI thread.
The latest commit has really minimized the UI thread use; just once per the unique RFH, which takes place only in the first setPreferredSinkId request.
Please see my previous comments: what I'm asking is to move authorization handler creation to IO thread. There is nothing preventing it, no matter what needs to be done on UI thread prior to that.
thanks, done with passing MediaStreamManager to it.
Related details
- Guido Urdaneta
- 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 20 comments![]( "Open in Gerrit")
GlobalRenderFrameHostId main_frame_id_for_testing_;The role of this is not clear. Why do we need it , if we can provide a custom authorization_handler_for_testing_ which could reject whatever frame we want to reject?
std::map</*main RFH id=*/GlobalRenderFrameHostId, PreferredSinkIdToSwitcher>This does not need to be a map, as main RFH id is a part of "PreferredSinkIdToSwitcher". So set is enough. (Otherwise we store main RFH id twice)
std::vector<DeviceSwitcher> active_device_switchers_;Why are they called "active"? Are there any passive ones?
GlobalRenderFrameHostId main_frame_global_id_;const, and move it up?
preferred_sink_id_ = sink_id;Shouldn't the setter also update the switchers if necessary?
PreferredSinkIdToSwitcher();Should take main_frame_global_id?
raw_ptr<AudioOutputDeviceSwitcher> output_device_switcher;"native_frame_id" info was a part of the switcher API at some point, and that was a bit more compact and easier to read. OOC: Why do you want to track it externally?
class PreferredSinkIdToSwitcher {Naming is not very descriptive. Maybe MainFramePreferredSinkIdConfig, or something along the line? (it's not SinkId to a switcher, there are multiple switcher)
const GlobalRenderFrameHostId& GetMainRenderFrameHostId(Since the whole API it passing GlobalRenderFrameHostId by value, we can return it by value here as well (I think it was me who suggested to return const ref - sorry for misleading)
// The methods are virtual for testing purpose.Since all public methods are virtual, could you split it into an interface and an implementation?
std::unique_ptr<AudioOutputAuthorizationHandler> authorization_handler;
if (authorization_handler_for_testing_) {
authorization_handler = std::move(authorization_handler_for_testing_);
}Move after "if" below and adjust? According to the main logic, authorization handler is only created for a non-default device.
Also, this approach allows for only one call to SetPreferredSinkId(), unless there's another SetAuthorizationForTesting() call. Please document that for SetAuthorizationForTesting() in the header.
A cleaner way would be to inject into the constructor an optional callback "create authorization handler for testing".
// The cache already exists, it will update the preferred sink id for theIf?
const GlobalRenderFrameHostId& main_frame_id =GlobalRenderFrameHostId
const GlobalRenderFrameHostId& main_frame_id =GlobalRenderFrameHostId
const GlobalRenderFrameHostId& main_frame_id =value, not const ref
preferred_sink_id_switchers_.erase(main_frame_id);DCHECK as well?
PreferredAudioOutputDeviceManager::GetMainRenderFrameHostId(MaybeGetCachedMainRenderFrameHostId()
and document that it will return an empty id if there is no entry
AudioOutputDeviceSwitcher* device_switcher) {It always seems to be null, why do we need it?
Related details
- Guido Urdaneta
- 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 23 comments![]( "Open in Gerrit")
Sunggook Chue1) Please fix the CL chain, and keep it up to date for all next review rounds.
2) Please restore the design doc and close it for public editing.
Olga SharonovaThe latest CL are 4 and it is up to date, I abandoned not used ones. please let me know if there is anything else I need to do.
I repaired the doc, I restricted the commentor with invite based from link.
Sunggook ChueThanks.
The CL chain needs to be rebased, now all follow-up CLs are "indirect relation"
I created the next branch with -b command, and --set-upstream-to=localBranch. it works fine in terms of 'fetch' command.
Based on research, i tried to convert indirect to direct by resetting --set-upstream-to=origin/localBranch instead of --set-upstream-to=localBranch, it failed due to missing origin/localBranch. will researching it.
It looks indirect is disappear for me. please reactive it if it is shown to you.
The role of this is not clear. Why do we need it , if we can provide a custom authorization_handler_for_testing_ which could reject whatever frame we want to reject?
yes, we can delete it. thanks.
std::map</*main RFH id=*/GlobalRenderFrameHostId, PreferredSinkIdToSwitcher>This does not need to be a map, as main RFH id is a part of "PreferredSinkIdToSwitcher". So set is enough. (Otherwise we store main RFH id twice)
we can use vector here.
std::vector<DeviceSwitcher> active_device_switchers_;Why are they called "active"? Are there any passive ones?
good questions, no passive. updated to simply 'device_switchers'.
const, and move it up?
Done
preferred_sink_id_ = sink_id;Shouldn't the setter also update the switchers if necessary?
switcher is updated by active_device_switchers() directly because device_swithcer could be added multiple times for single 'preferred sink id' ('single PreferredSinkIdToSwitcher will have single 'preferred sink id').
PreferredSinkIdToSwitcher();Sunggook ChueShould take main_frame_global_id?
thanks,
raw_ptr<AudioOutputDeviceSwitcher> output_device_switcher;"native_frame_id" info was a part of the switcher API at some point, and that was a bit more compact and easier to read. OOC: Why do you want to track it externally?
Initially it was private, but active_device_switchers() is public that returns the structure so move the structure to the public as 'native frame id' and 'device switcher' should be paired.
If we really keep it under private, then we can modify the active_device_switchers().
MainFramePreferredSinkIdConfig.AddSwitcher('main frame id', 'native frame id', 'device switcher object').
MainFramePreferredSinkIdConfig.GetDeviceSwitchers('main frame id'): The implementation has to create new vector of 'device switcher object' from the internal std::vector<DeviceSwitcher>.
The alternative seems more complex to understand the code. maybe other way to implement, any suggestion?
Naming is not very descriptive. Maybe MainFramePreferredSinkIdConfig, or something along the line? (it's not SinkId to a switcher, there are multiple switcher)
MainFramePreferredSinkIdConfig sounds good. thanks.
const GlobalRenderFrameHostId& GetMainRenderFrameHostId(Since the whole API it passing GlobalRenderFrameHostId by value, we can return it by value here as well (I think it was me who suggested to return const ref - sorry for misleading)
Done
Since all public methods are virtual, could you split it into an interface and an implementation?
Done
// `RenderFrameAudioOutputStreamFactory::Core::Init()` and deletion of theSunggook ChueIs the code there?
(Also please do not refer to the external context)
Olga Sharonovabut you know it is correct if you see the next CL? I thought the benefit of these relation CL is to know the future work so we can have complete class that doesn't have to updated the next CL.
anyway, I will remove this one.
Sunggook ChueIt was not in the next CL. and the CL chain is broken.
Olga Sharonovathe next CL is https://chromium-review.googlesource.com/c/chromium/src/+/5814796/6. Please ignore any gray color (abandoned), somehow gerrit does not remove it automatically, maybe I have to unset and upload for the abandoned, I will try it.
Sunggook ChueHave you tried
I tried, git cl upload command does not work yet all as it said it is abandoned. It is my first time of using relation CLs, many mistakes, sorry about that.
It looks indirect is disappear for me. please reactive it if it is shown to you.
std::unique_ptr<AudioOutputAuthorizationHandler> authorization_handler;
if (authorization_handler_for_testing_) {
authorization_handler = std::move(authorization_handler_for_testing_);
}Move after "if" below and adjust? According to the main logic, authorization handler is only created for a non-default device.
Also, this approach allows for only one call to SetPreferredSinkId(), unless there's another SetAuthorizationForTesting() call. Please document that for SetAuthorizationForTesting() in the header.
A cleaner way would be to inject into the constructor an optional callback "create authorization handler for testing".
I follow the above 2 suggestions, but adding optional para for the testing code touched the product code where I'd like to avoid if possible; testing code is isolated clearly as much as possible.
I resolve this one, please reactive if you think adding optional param is better.
// The cache already exists, it will update the preferred sink id for theSunggook ChueIf?
Done
const GlobalRenderFrameHostId& main_frame_id =Sunggook ChueGlobalRenderFrameHostId
use const GlobalRenderFrameHostId.
Sunggook ChueWhat if (!main_frame_id)?
it should exist at this time if the contract is correct, adding CHECK.
const GlobalRenderFrameHostId& main_frame_id =Sunggook ChueGlobalRenderFrameHostId
const GlobalRenderFrameHostId
const GlobalRenderFrameHostId& main_frame_id =Sunggook Chuevalue, not const ref
Done
Sunggook ChueWhat if (!main_frame_id)?
It failed on the preferred_sink_id_switchers_.find(main_frame_id) or (new API of FindSinkIdConfig) that does nothing.
preferred_sink_id_switchers_.erase(main_frame_id);Sunggook ChueDCHECK as well?
no, preferred_sink_id_switchers_ has an entry only when SetSink* API is called or AddSwticher is called. but UnregisterMainFrameOnIOThread will be called when RFH is deleted regardless of these APIs are called.
Please reactive it if it is different from your understanding.
PreferredAudioOutputDeviceManager::GetMainRenderFrameHostId(MaybeGetCachedMainRenderFrameHostId()
and document that it will return an empty id if there is no entry
Done
It always seems to be null, why do we need it?
no, if it is also called from the AddSwitcher where the value exists.
Related details
- Guido Urdaneta
- 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 1 comment![]( "Open in Gerrit")
Please unmark as "resolved" the answers to the questions which have meanings conceptually different from "done".
Related details
- Guido Urdaneta
- 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. |
Olga Sharonova (Gerrit)
Olga Sharonova added 1 comment![]( "Open in Gerrit")
And please update the CL chain.
Sunggook Chue (Gerrit)
Sunggook Chue added 3 comments![]( "Open in Gerrit")
Sunggook ChuePlease unmark as "resolved" the answers to the questions which have meanings conceptually different from "done".
I just uploaded this CL, not the next so it looks it causes indirection on this relation.
upload next CLs, then its indirection disappeared.
And please update the CL chain.
Done
- Guido Urdaneta
- 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. |
Sunggook Chue (Gerrit)
Sunggook Chue added 1 comment![]( "Open in Gerrit")
// The methods are virtual for testing purpose.Sunggook ChueSince all public methods are virtual, could you split it into an interface and an implementation?
Done
Interface looks meaning there are multiple implementations for the feature (manager), but not for us. I don't see many media side code example that introduced the interface for the Mock test only even they Mock is common for test.
If you still think interface is good, I could support it though.
Olga Sharonova (Gerrit)
Olga Sharonova added 1 comment![]( "Open in Gerrit")
// The methods are virtual for testing purpose.Sunggook ChueSince all public methods are virtual, could you split it into an interface and an implementation?
Sunggook ChueDone
Interface looks meaning there are multiple implementations for the feature (manager), but not for us. I don't see many media side code example that introduced the interface for the Mock test only even they Mock is common for test.
If you still think interface is good, I could support it though.
The fact that you made it all virtual for testing means that you already have at least two implementations, doesn't it?
It's a common practice in chromium when an object is an external dependency and needs to be mocked. And it's cleaner than making all the methods virtual for testing and engaging in implementation inheritance.
Related details
- Guido Urdaneta
- 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 2 comments![]( "Open in Gerrit")
// The methods are virtual for testing purpose.Sunggook ChueSince all public methods are virtual, could you split it into an interface and an implementation?
Sunggook ChueDone
Olga SharonovaInterface looks meaning there are multiple implementations for the feature (manager), but not for us. I don't see many media side code example that introduced the interface for the Mock test only even they Mock is common for test.
If you still think interface is good, I could support it though.
The fact that you made it all virtual for testing means that you already have at least two implementations, doesn't it?
It's a common practice in chromium when an object is an external dependency and needs to be mocked. And it's cleaner than making all the methods virtual for testing and engaging in implementation inheritance.
One of concern I had was that we have to force the Mock class to provide all virtual methods, which can bloat the test code where it Mock only single method now.
If we use single file for Mock, then it would be harder for selective Mock methods to have different implementation by test requirement.
so, even if we provide an interface, the test code will have to Mock the AudioOutputAuthorizationHandler directly. updated.
Related details
- Guido Urdaneta
- 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 2 comments![]( "Open in Gerrit")
raw_ptr<AudioOutputDeviceSwitcher> output_device_switcher;Sunggook Chue"native_frame_id" info was a part of the switcher API at some point, and that was a bit more compact and easier to read. OOC: Why do you want to track it externally?
Initially it was private, but active_device_switchers() is public that returns the structure so move the structure to the public as 'native frame id' and 'device switcher' should be paired.
If we really keep it under private, then we can modify the active_device_switchers().
MainFramePreferredSinkIdConfig.AddSwitcher('main frame id', 'native frame id', 'device switcher object').
MainFramePreferredSinkIdConfig.GetDeviceSwitchers('main frame id'): The implementation has to create new vector of 'device switcher object' from the internal std::vector<DeviceSwitcher>.
The alternative seems more complex to understand the code. maybe other way to implement, any suggestion?
What do you refer to as "it was private"?
What I mean is, if I remember correctly, the device switcher interface had the frame ID as a getter at some point. AudioOutputDeviceSwitcher can have GetOwnFrameId() (so DeviceSwitcher struct won't be needed) - and even GetMainFrameId() as well if we wish so.
// The methods are virtual for testing purpose.Sunggook ChueSince all public methods are virtual, could you split it into an interface and an implementation?
Sunggook ChueDone
Olga SharonovaInterface looks meaning there are multiple implementations for the feature (manager), but not for us. I don't see many media side code example that introduced the interface for the Mock test only even they Mock is common for test.
If you still think interface is good, I could support it though.
Sunggook ChueThe fact that you made it all virtual for testing means that you already have at least two implementations, doesn't it?
It's a common practice in chromium when an object is an external dependency and needs to be mocked. And it's cleaner than making all the methods virtual for testing and engaging in implementation inheritance.
One of concern I had was that we have to force the Mock class to provide all virtual methods, which can bloat the test code where it Mock only single method now.
If we use single file for Mock, then it would be harder for selective Mock methods to have different implementation by test requirement.
so, even if we provide an interface, the test code will have to Mock the AudioOutputAuthorizationHandler directly. updated.
Could you please place interface and Impl into the same header file (it's fine), and rename all the files back? Gerrit does not see similarities after this rename, so I can't see the diff (here: https://chromium-review.googlesource.com/c/chromium/src/+/5664173/33..36) and can't do an incremental review.
Could you clarify "If we use single file for Mock" part - what do you mean?
There can be a common mock for multiple test suits if convenient, but also tests suits can customize that mock and write their own.
Related details
- Guido Urdaneta
- 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")
Sunggook ChuePlease unmark as "resolved" the answers to the questions which have meanings conceptually different from "done".
I just uploaded this CL, not the next so it looks it causes indirection on this relation.
upload next CLs, then its indirection disappeared.
Acknowledged
preferred_sink_id_ = sink_id;Sunggook ChueShouldn't the setter also update the switchers if necessary?
switcher is updated by active_device_switchers() directly because device_swithcer could be added multiple times for single 'preferred sink id' ('single PreferredSinkIdToSwitcher will have single 'preferred sink id').
Please reactivate it if my above explanation isn't sufficient. thanks.
raw_ptr<AudioOutputDeviceSwitcher> output_device_switcher;Sunggook Chue"native_frame_id" info was a part of the switcher API at some point, and that was a bit more compact and easier to read. OOC: Why do you want to track it externally?
Olga SharonovaInitially it was private, but active_device_switchers() is public that returns the structure so move the structure to the public as 'native frame id' and 'device switcher' should be paired.
If we really keep it under private, then we can modify the active_device_switchers().
MainFramePreferredSinkIdConfig.AddSwitcher('main frame id', 'native frame id', 'device switcher object').
MainFramePreferredSinkIdConfig.GetDeviceSwitchers('main frame id'): The implementation has to create new vector of 'device switcher object' from the internal std::vector<DeviceSwitcher>.
The alternative seems more complex to understand the code. maybe other way to implement, any suggestion?
What do you refer to as "it was private"?
What I mean is, if I remember correctly, the device switcher interface had the frame ID as a getter at some point. AudioOutputDeviceSwitcher can have GetOwnFrameId() (so DeviceSwitcher struct won't be needed) - and even GetMainFrameId() as well if we wish so.
I'm confused, can you help me understand?
'native_frame_id' in the DeviceSwitcher isn't required in terms of functionality but it is introduced for better readability as your feedback.
We can get 'native_frame_id' if we enforce the implementor of the AudioOutputDeviceSwitcher to provide 'GetOwnFrameId'. But this will add unnecessary burden to the implementor even when it isn't used for functionality, and it also hide readability that was initial feedback?
maybe I misunderstood your initial feedback ""native_frame_id" info was a part of the switcher API at some point, and that was a bit more compact and easier to read.".
If you're okay, we can remove "native_frame_id" and DeviceSwitcher yet all from this class and no GetOwnFrameId yet all.
// The methods are virtual for testing purpose.Sunggook ChueSince all public methods are virtual, could you split it into an interface and an implementation?
Sunggook ChueDone
Olga SharonovaInterface looks meaning there are multiple implementations for the feature (manager), but not for us. I don't see many media side code example that introduced the interface for the Mock test only even they Mock is common for test.
If you still think interface is good, I could support it though.
Sunggook ChueThe fact that you made it all virtual for testing means that you already have at least two implementations, doesn't it?
It's a common practice in chromium when an object is an external dependency and needs to be mocked. And it's cleaner than making all the methods virtual for testing and engaging in implementation inheritance.
Olga SharonovaOne of concern I had was that we have to force the Mock class to provide all virtual methods, which can bloat the test code where it Mock only single method now.
If we use single file for Mock, then it would be harder for selective Mock methods to have different implementation by test requirement.
so, even if we provide an interface, the test code will have to Mock the AudioOutputAuthorizationHandler directly. updated.
Could you please place interface and Impl into the same header file (it's fine), and rename all the files back? Gerrit does not see similarities after this rename, so I can't see the diff (here: https://chromium-review.googlesource.com/c/chromium/src/+/5664173/33..36) and can't do an incremental review.
Could you clarify "If we use single file for Mock" part - what do you mean?
There can be a common mock for multiple test suits if convenient, but also tests suits can customize that mock and write their own.
yes, I mentioned common mock, but looks didn't provide any value as each test customize the mock (no common mock is used). we avoid this common mock by mocking the directly the PreferredAudioOutputDeviceManagerImpl.
I update to merge the file to single one. thanks.
Related details
- Guido Urdaneta
- 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 7 comments![]( "Open in Gerrit")
std::vector<DeviceSwitcher>& device_switchers() {Please remove it and introduce a proper API: add / update sink/ clear - whatever.
preferred_sink_id_ = sink_id;Sunggook ChueShouldn't the setter also update the switchers if necessary?
Sunggook Chueswitcher is updated by active_device_switchers() directly because device_swithcer could be added multiple times for single 'preferred sink id' ('single PreferredSinkIdToSwitcher will have single 'preferred sink id').
Please reactivate it if my above explanation isn't sufficient. thanks.
I've asked earlier on many occasions to not resolve the comments which are discussions. I will resolve them when I'm satisfied with the outcome of the discussion. Resolved comments, as I explain, get lost, and it's a waste of my time to recall a discussion and search for it.
I won't be reviewing at all the uploads which do not satisfy this request. Thank you for understanding.
---
I don't understand this explanation, please see the other comments.
raw_ptr<AudioOutputDeviceSwitcher> output_device_switcher;Sunggook Chue"native_frame_id" info was a part of the switcher API at some point, and that was a bit more compact and easier to read. OOC: Why do you want to track it externally?
Olga SharonovaInitially it was private, but active_device_switchers() is public that returns the structure so move the structure to the public as 'native frame id' and 'device switcher' should be paired.
If we really keep it under private, then we can modify the active_device_switchers().
MainFramePreferredSinkIdConfig.AddSwitcher('main frame id', 'native frame id', 'device switcher object').
MainFramePreferredSinkIdConfig.GetDeviceSwitchers('main frame id'): The implementation has to create new vector of 'device switcher object' from the internal std::vector<DeviceSwitcher>.
The alternative seems more complex to understand the code. maybe other way to implement, any suggestion?
Sunggook ChueWhat do you refer to as "it was private"?
What I mean is, if I remember correctly, the device switcher interface had the frame ID as a getter at some point. AudioOutputDeviceSwitcher can have GetOwnFrameId() (so DeviceSwitcher struct won't be needed) - and even GetMainFrameId() as well if we wish so.
I'm confused, can you help me understand?
'native_frame_id' in the DeviceSwitcher isn't required in terms of functionality but it is introduced for better readability as your feedback.
We can get 'native_frame_id' if we enforce the implementor of the AudioOutputDeviceSwitcher to provide 'GetOwnFrameId'. But this will add unnecessary burden to the implementor even when it isn't used for functionality, and it also hide readability that was initial feedback?
maybe I misunderstood your initial feedback ""native_frame_id" info was a part of the switcher API at some point, and that was a bit more compact and easier to read.".
If you're okay, we can remove "native_frame_id" and DeviceSwitcher yet all from this class and no GetOwnFrameId yet all.
It must be some misunderstanding: I have not proposed to store it just for fun - the mapping was there. If the mapping between switcher and its frame id is not needed, feel free to remove it.
To be clear, what I'm proposing here is to change the API to:
virtual void AddSwitcher(AudioOutputDeviceSwitcher* device_switcher) = 0;
virtual void RemoveSwitcher(AudioOutputDeviceSwitcher* device_switcher) = 0;
in case you need to look up a native frame id for the switcher. If you don't - ignore it.
// The methods are virtual for testing purpose.Sunggook ChueSince all public methods are virtual, could you split it into an interface and an implementation?
Sunggook ChueDone
Olga SharonovaInterface looks meaning there are multiple implementations for the feature (manager), but not for us. I don't see many media side code example that introduced the interface for the Mock test only even they Mock is common for test.
If you still think interface is good, I could support it though.
Sunggook ChueThe fact that you made it all virtual for testing means that you already have at least two implementations, doesn't it?
It's a common practice in chromium when an object is an external dependency and needs to be mocked. And it's cleaner than making all the methods virtual for testing and engaging in implementation inheritance.
Olga SharonovaOne of concern I had was that we have to force the Mock class to provide all virtual methods, which can bloat the test code where it Mock only single method now.
If we use single file for Mock, then it would be harder for selective Mock methods to have different implementation by test requirement.
so, even if we provide an interface, the test code will have to Mock the AudioOutputAuthorizationHandler directly. updated.
Sunggook ChueCould you please place interface and Impl into the same header file (it's fine), and rename all the files back? Gerrit does not see similarities after this rename, so I can't see the diff (here: https://chromium-review.googlesource.com/c/chromium/src/+/5664173/33..36) and can't do an incremental review.
Could you clarify "If we use single file for Mock" part - what do you mean?
There can be a common mock for multiple test suits if convenient, but also tests suits can customize that mock and write their own.
yes, I mentioned common mock, but looks didn't provide any value as each test customize the mock (no common mock is used). we avoid this common mock by mocking the directly the PreferredAudioOutputDeviceManagerImpl.
I update to merge the file to single one. thanks.
" we avoid this common mock" - what's the benefit of it?
Mock class is very simple. Also the mocked methods you have now in the follow-up do not have any behavior defined, so you can just as well introduce a common mock.
Even if a specific method needs to be customized, you can override it for the test.
Please do not inherit from PreferredAudioOutputDeviceManagerImpl for mocking.
if (config->preferred_sink_id() != raw_device_id) {
for (auto& device_switcher : config->device_switchers()) {
device_switcher.output_device_switcher->SwitchAudioOutputDeviceId(
raw_device_id);
}This logic should be in set_preferred_sink_id() below
preferred_sink_id_config->device_switchers().emplace_back(native_frame_id,shouldn't the sink be switched for the switcher?
Related details
- Guido Urdaneta
- 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 6 comments![]( "Open in Gerrit")
Please remove it and introduce a proper API: add / update sink/ clear - whatever.
replaced, thanks.
preferred_sink_id_ = sink_id;Sunggook ChueShouldn't the setter also update the switchers if necessary?
Sunggook Chueswitcher is updated by active_device_switchers() directly because device_swithcer could be added multiple times for single 'preferred sink id' ('single PreferredSinkIdToSwitcher will have single 'preferred sink id').
Olga SharonovaPlease reactivate it if my above explanation isn't sufficient. thanks.
I've asked earlier on many occasions to not resolve the comments which are discussions. I will resolve them when I'm satisfied with the outcome of the discussion. Resolved comments, as I explain, get lost, and it's a waste of my time to recall a discussion and search for it.
I won't be reviewing at all the uploads which do not satisfy this request. Thank you for understanding.
---
I don't understand this explanation, please see the other comments.
Config has (1) preferred sink id (2) device switcher list for 'main frame id'.
The inserting of (1) and (2) comes from the different times (SetPreferredSinkId for (1), and AddSwitcher for (2).
So, there is no need to edit the (2) during SetPreferredSinkId API call.
raw_ptr<AudioOutputDeviceSwitcher> output_device_switcher;Sunggook Chue"native_frame_id" info was a part of the switcher API at some point, and that was a bit more compact and easier to read. OOC: Why do you want to track it externally?
Olga SharonovaInitially it was private, but active_device_switchers() is public that returns the structure so move the structure to the public as 'native frame id' and 'device switcher' should be paired.
If we really keep it under private, then we can modify the active_device_switchers().
MainFramePreferredSinkIdConfig.AddSwitcher('main frame id', 'native frame id', 'device switcher object').
MainFramePreferredSinkIdConfig.GetDeviceSwitchers('main frame id'): The implementation has to create new vector of 'device switcher object' from the internal std::vector<DeviceSwitcher>.
The alternative seems more complex to understand the code. maybe other way to implement, any suggestion?
Sunggook ChueWhat do you refer to as "it was private"?
What I mean is, if I remember correctly, the device switcher interface had the frame ID as a getter at some point. AudioOutputDeviceSwitcher can have GetOwnFrameId() (so DeviceSwitcher struct won't be needed) - and even GetMainFrameId() as well if we wish so.
Olga SharonovaI'm confused, can you help me understand?
'native_frame_id' in the DeviceSwitcher isn't required in terms of functionality but it is introduced for better readability as your feedback.
We can get 'native_frame_id' if we enforce the implementor of the AudioOutputDeviceSwitcher to provide 'GetOwnFrameId'. But this will add unnecessary burden to the implementor even when it isn't used for functionality, and it also hide readability that was initial feedback?
maybe I misunderstood your initial feedback ""native_frame_id" info was a part of the switcher API at some point, and that was a bit more compact and easier to read.".
If you're okay, we can remove "native_frame_id" and DeviceSwitcher yet all from this class and no GetOwnFrameId yet all.
It must be some misunderstanding: I have not proposed to store it just for fun - the mapping was there. If the mapping between switcher and its frame id is not needed, feel free to remove it.
To be clear, what I'm proposing here is to change the API to:
virtual void AddSwitcher(AudioOutputDeviceSwitcher* device_switcher) = 0;
virtual void RemoveSwitcher(AudioOutputDeviceSwitcher* device_switcher) = 0;in case you need to look up a native frame id for the switcher. If you don't - ignore it.
thanks for clarification, I will ignore it as we don't use it.
// The methods are virtual for testing purpose.Done
if (config->preferred_sink_id() != raw_device_id) {
for (auto& device_switcher : config->device_switchers()) {
device_switcher.output_device_switcher->SwitchAudioOutputDeviceId(
raw_device_id);
}This logic should be in set_preferred_sink_id() below
set_preferred_sink_id(raw_device_id) updates the sink_id, then it will make config->preferred_sink_id() is always raw_device_id.
I will put the 'for loop' below the set_preferred_sink_id that might be clearer for the code.
preferred_sink_id_config->device_switchers().emplace_back(native_frame_id,shouldn't the sink be switched for the switcher?
can you clarify the question?
Config has (1) single preferred sink id and (2) multiple switchers. The above line just adding one of switcher when it was requested from the AddSwitcher.
Related details
- Guido Urdaneta
- 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")
std::vector<DeviceSwitcher>& device_switchers() {Sunggook ChuePlease remove it and introduce a proper API: add / update sink/ clear - whatever.
replaced, thanks.
Please change it so that Config manages switchers, rather than the external code. I.e. no GetDeviceSwitcherByIndex().
if (config->preferred_sink_id() != raw_device_id) {
for (auto& device_switcher : config->device_switchers()) {
device_switcher.output_device_switcher->SwitchAudioOutputDeviceId(
raw_device_id);
}Sunggook ChueThis logic should be in set_preferred_sink_id() below
set_preferred_sink_id(raw_device_id) updates the sink_id, then it will make config->preferred_sink_id() is always raw_device_id.
I will put the 'for loop' below the set_preferred_sink_id that might be clearer for the code.
Sorry, I don't understand what is written here.
Config should be responsible for calling SwitchAudioOutputDeviceId() for its switchers. There should be no GetDeviceSwitcherByIndex() method.
preferred_sink_id_config->device_switchers().emplace_back(native_frame_id,Sunggook Chueshouldn't the sink be switched for the switcher?
can you clarify the question?
Config has (1) single preferred sink id and (2) multiple switchers. The above line just adding one of switcher when it was requested from the AddSwitcher.
Switchers added to a config should all have the output device controlled by SwitchAudioOutputDeviceId(). You add a switcher but do not update its output device to sink_id.
Related details
- Guido Urdaneta
- 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")
std::vector<DeviceSwitcher>& device_switchers() {Sunggook ChuePlease remove it and introduce a proper API: add / update sink/ clear - whatever.
Olga Sharonovareplaced, thanks.
Please change it so that Config manages switchers, rather than the external code. I.e. no GetDeviceSwitcherByIndex().
got it, thanks. updated.
if (config->preferred_sink_id() != raw_device_id) {
for (auto& device_switcher : config->device_switchers()) {
device_switcher.output_device_switcher->SwitchAudioOutputDeviceId(
raw_device_id);
}Sunggook ChueThis logic should be in set_preferred_sink_id() below
Olga Sharonovaset_preferred_sink_id(raw_device_id) updates the sink_id, then it will make config->preferred_sink_id() is always raw_device_id.
I will put the 'for loop' below the set_preferred_sink_id that might be clearer for the code.
Sorry, I don't understand what is written here.
Config should be responsible for calling SwitchAudioOutputDeviceId() for its switchers. There should be no GetDeviceSwitcherByIndex() method.
updated,
preferred_sink_id_config->device_switchers().emplace_back(native_frame_id,Sunggook Chueshouldn't the sink be switched for the switcher?
Olga Sharonovacan you clarify the question?
Config has (1) single preferred sink id and (2) multiple switchers. The above line just adding one of switcher when it was requested from the AddSwitcher.
Switchers added to a config should all have the output device controlled by SwitchAudioOutputDeviceId(). You add a switcher but do not update its output device to sink_id.
It calls SwitchAudioOutputDeviceId under the for loop for switchers, but as you suggested, move this to Config with new method of UpdateSinkIdForSwitchersIfNeeded().
Related details
- Guido Urdaneta
- 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 11 comments![]( "Open in Gerrit")
preferred_sink_id_switchers_;Why is it still called like that? preferred_sink_id_configs_?
// The class to store the preferred sink id for the render frame host and its
// all sub frames. For this purpose, it stores audio device switchers
// as well as preferred sink id by the main render frame host id.This class controls preferred sink id for all switchers registered as belonging to a tree of a given main frame.
---
"it stores audio device switchers as well as preferred sink id by the main render frame host id." is written in c++ below, no need to add it to the comment.
GlobalRenderFrameHostId main_frame_global_id);make it take sink_id as a parameter
void SetPreferredSinkId(const std::string& sink_id) {I tried to explain several times: this method is pretty useless as stand-alone, please remove it.
The contract of the class is all added switchers are rendering to the current prefered sink. This partial methods break the contract.
device_switchers_.push_back(device_switcher);device_switcher->SwitchAudioOutputDeviceId(preferred_sink_id_);
size_t GetDeviceSwitcherCount() const { return device_switchers_.size(); }IsEmpty()
void MaybeUpdateSinkIdForSwitchers(const std::string& new_device_id) {It's enough to call it SetPreferredSinkId(). Updating switchers is a part of the contract.
UnregisterMainFrameOnIOThread(main_frame_id);Is it correct? What if SetPreferredSinkId is called, then a switcher is added, then it's removed, then a new one is added? The information about preferred sink is lost, new switcher's device id won't be updated.
(You also need a unit test for that case.)
preferred_sink_id_config->device_switchers().emplace_back(native_frame_id,Sunggook Chueshouldn't the sink be switched for the switcher?
Olga Sharonovacan you clarify the question?
Config has (1) single preferred sink id and (2) multiple switchers. The above line just adding one of switcher when it was requested from the AddSwitcher.
Sunggook ChueSwitchers added to a config should all have the output device controlled by SwitchAudioOutputDeviceId(). You add a switcher but do not update its output device to sink_id.
It calls SwitchAudioOutputDeviceId under the for loop for switchers, but as you suggested, move this to Config with new method of UpdateSinkIdForSwitchersIfNeeded().
Which loop are we talking about here? You just added a switcher and the sink is not updated. Where is the update happening?
config->SetPreferredSinkId(sink_id);pass sink_id into constructor?
Related details
- Guido Urdaneta
- 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 12 comments![]( "Open in Gerrit")
Why is it still called like that? preferred_sink_id_configs_?
renamed preferred_sink_id_config_.
// The class to store the preferred sink id for the render frame host and its
// all sub frames. For this purpose, it stores audio device switchers
// as well as preferred sink id by the main render frame host id.This class controls preferred sink id for all switchers registered as belonging to a tree of a given main frame.
---
"it stores audio device switchers as well as preferred sink id by the main render frame host id." is written in c++ below, no need to add it to the comment.
removed the latter part.
make it take sink_id as a parameter
Done
void SetPreferredSinkId(const std::string& sink_id) {I tried to explain several times: this method is pretty useless as stand-alone, please remove it.
The contract of the class is all added switchers are rendering to the current prefered sink. This partial methods break the contract.
We have to update the sink_id at any time by the call of 'SetPreferredSinkId' from the JS side, but not we created MaybeUpdateSinkIdForSwitchers so we can remove it now. updated.
device_switchers_.push_back(device_switcher);device_switcher->SwitchAudioOutputDeviceId(preferred_sink_id_);
calling device_switcher->SwitchAudioOutputDeviceId(preferred_sink_id_) does not work because 'AddSwitcher' is called before stream is created from the AudioOutputStreamBroker::CreateStream. If needed, the caller (i.e. AudioOutputStreamBroker::CreateStream) can call SwitchAudioOutputDeviceId directly after comparing GetPreferredSinkId. btw, The previous API was a single call AddAndGet*, but splitting into two (GetPreferredSinkId/AddSwitcher) that provide cleaner interface for the client.
I updated the comment of 'AddSwitcher'.
size_t GetDeviceSwitcherCount() const { return device_switchers_.size(); }Sunggook ChueIsEmpty()
Done
void MaybeUpdateSinkIdForSwitchers(const std::string& new_device_id) {It's enough to call it SetPreferredSinkId(). Updating switchers is a part of the contract.
I removed SetPreferredSinkId() instead, and will change this method name to the MaybeSetPreferredSinkId since if the sink_id is same with the old one, there is no action.
SetPreferredSinkId(new_device_id);Sunggook Chueremove
update it directly, remove SetPreferredSinkId
Is it correct? What if SetPreferredSinkId is called, then a switcher is added, then it's removed, then a new one is added? The information about preferred sink is lost, new switcher's device id won't be updated.
(You also need a unit test for that case.)
You're right, we have to Unregister only when the config's sink id is 'default'. thanks.
preferred_sink_id_config->device_switchers().emplace_back(native_frame_id,Sunggook Chueshouldn't the sink be switched for the switcher?
Olga Sharonovacan you clarify the question?
Config has (1) single preferred sink id and (2) multiple switchers. The above line just adding one of switcher when it was requested from the AddSwitcher.
Sunggook ChueSwitchers added to a config should all have the output device controlled by SwitchAudioOutputDeviceId(). You add a switcher but do not update its output device to sink_id.
Olga SharonovaIt calls SwitchAudioOutputDeviceId under the for loop for switchers, but as you suggested, move this to Config with new method of UpdateSinkIdForSwitchersIfNeeded().
Which loop are we talking about here? You just added a switcher and the sink is not updated. Where is the update happening?
I explained on the AddDeviceSwitcher comment, SwitchAudioOutputDeviceId shouldn't be called during AddSwitcher.
config->SetPreferredSinkId(sink_id);Sunggook Chuepass sink_id into constructor?
Done
Related details
- Guido Urdaneta
- 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 6 comments![]( "Open in Gerrit")
preferred_sink_id_switchers_;Sunggook ChueWhy is it still called like that? preferred_sink_id_configs_?
renamed preferred_sink_id_config_.
preferred_sink_id_configs_? (it's a vector)
// The class to store the preferred sink id for the render frame host and its
// all sub frames. For this purpose, it stores audio device switchers
// as well as preferred sink id by the main render frame host id.Sunggook ChueThis class controls preferred sink id for all switchers registered as belonging to a tree of a given main frame.
---
"it stores audio device switchers as well as preferred sink id by the main render frame host id." is written in c++ below, no need to add it to the comment.
removed the latter part.
Could you please fix the comment. What is store is not interesting, it's important what it is responsible for.
device_switchers_.push_back(device_switcher);Sunggook Chuedevice_switcher->SwitchAudioOutputDeviceId(preferred_sink_id_);
calling device_switcher->SwitchAudioOutputDeviceId(preferred_sink_id_) does not work because 'AddSwitcher' is called before stream is created from the AudioOutputStreamBroker::CreateStream. If needed, the caller (i.e. AudioOutputStreamBroker::CreateStream) can call SwitchAudioOutputDeviceId directly after comparing GetPreferredSinkId. btw, The previous API was a single call AddAndGet*, but splitting into two (GetPreferredSinkId/AddSwitcher) that provide cleaner interface for the client.
I updated the comment of 'AddSwitcher'.
There is nothing in the switcher contract about that, so the switcher implementation needs to be fixed to fulfill its contract, rather than we adapt to implementation details of the interface.
void MaybeUpdateSinkIdForSwitchers(const std::string& new_device_id) {Sunggook ChueIt's enough to call it SetPreferredSinkId(). Updating switchers is a part of the contract.
I removed SetPreferredSinkId() instead, and will change this method name to the MaybeSetPreferredSinkId since if the sink_id is same with the old one, there is no action.
Please remove "Maybe", it's not normally used when the action is not performed due to a redundancy.
SetPreferredSinkId(new_device_id);Sunggook Chueremove
update it directly, remove SetPreferredSinkId
Move it before "for"? Usually the value is set, and then the update is done.
UnregisterMainFrameOnIOThread(main_frame_id);Sunggook ChueIs it correct? What if SetPreferredSinkId is called, then a switcher is added, then it's removed, then a new one is added? The information about preferred sink is lost, new switcher's device id won't be updated.
(You also need a unit test for that case.)
You're right, we have to Unregister only when the config's sink id is 'default'. thanks.
1) Which uint tests would cover this case?
2) I don't think we need to remove it at all. It will be removed when the main frame is gone, wouldn't it?
Related details
- Guido Urdaneta
- 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. |
Olga Sharonova (Gerrit)
Olga Sharonova added 1 comment![]( "Open in Gerrit")
See the other CL re: how to fix the switcher
Sunggook Chue (Gerrit)
Sunggook Chue added 8 comments![]( "Open in Gerrit")
preferred_sink_id_switchers_;Sunggook ChueWhy is it still called like that? preferred_sink_id_configs_?
Olga Sharonovarenamed preferred_sink_id_config_.
preferred_sink_id_configs_? (it's a vector)
Done
// The class to store the preferred sink id for the render frame host and its
// all sub frames. For this purpose, it stores audio device switchers
// as well as preferred sink id by the main render frame host id.Sunggook ChueThis class controls preferred sink id for all switchers registered as belonging to a tree of a given main frame.
---
"it stores audio device switchers as well as preferred sink id by the main render frame host id." is written in c++ below, no need to add it to the comment.
Olga Sharonovaremoved the latter part.
Could you please fix the comment. What is store is not interesting, it's important what it is responsible for.
updated "The class to provide switchers and preferred sink id for the main frame if
the preferred sink id is set for the main frame.".
please suggest if you have any others in mind?
device_switchers_.push_back(device_switcher);Sunggook Chuedevice_switcher->SwitchAudioOutputDeviceId(preferred_sink_id_);
Olga Sharonovacalling device_switcher->SwitchAudioOutputDeviceId(preferred_sink_id_) does not work because 'AddSwitcher' is called before stream is created from the AudioOutputStreamBroker::CreateStream. If needed, the caller (i.e. AudioOutputStreamBroker::CreateStream) can call SwitchAudioOutputDeviceId directly after comparing GetPreferredSinkId. btw, The previous API was a single call AddAndGet*, but splitting into two (GetPreferredSinkId/AddSwitcher) that provide cleaner interface for the client.
I updated the comment of 'AddSwitcher'.
There is nothing in the switcher contract about that, so the switcher implementation needs to be fixed to fulfill its contract, rather than we adapt to implementation details of the interface.
ok, then, I will SwitchAudioOutputDeviceId and see how it looks like.
void MaybeUpdateSinkIdForSwitchers(const std::string& new_device_id) {Sunggook ChueIt's enough to call it SetPreferredSinkId(). Updating switchers is a part of the contract.
Olga SharonovaI removed SetPreferredSinkId() instead, and will change this method name to the MaybeSetPreferredSinkId since if the sink_id is same with the old one, there is no action.
Please remove "Maybe", it's not normally used when the action is not performed due to a redundancy.
Done
SetPreferredSinkId(new_device_id);Sunggook Chueremove
Olga Sharonovaupdate it directly, remove SetPreferredSinkId
Move it before "for"? Usually the value is set, and then the update is done.
Done
UnregisterMainFrameOnIOThread(main_frame_id);Sunggook ChueIs it correct? What if SetPreferredSinkId is called, then a switcher is added, then it's removed, then a new one is added? The information about preferred sink is lost, new switcher's device id won't be updated.
(You also need a unit test for that case.)
Olga SharonovaYou're right, we have to Unregister only when the config's sink id is 'default'. thanks.
1) Which uint tests would cover this case?
2) I don't think we need to remove it at all. It will be removed when the main frame is gone, wouldn't it?
ok,
preferred_sink_id_config->device_switchers().emplace_back(native_frame_id,Sunggook Chueshouldn't the sink be switched for the switcher?
Olga Sharonovacan you clarify the question?
Config has (1) single preferred sink id and (2) multiple switchers. The above line just adding one of switcher when it was requested from the AddSwitcher.
Sunggook ChueSwitchers added to a config should all have the output device controlled by SwitchAudioOutputDeviceId(). You add a switcher but do not update its output device to sink_id.
Olga SharonovaIt calls SwitchAudioOutputDeviceId under the for loop for switchers, but as you suggested, move this to Config with new method of UpdateSinkIdForSwitchersIfNeeded().
Sunggook ChueWhich loop are we talking about here? You just added a switcher and the sink is not updated. Where is the update happening?
I explained on the AddDeviceSwitcher comment, SwitchAudioOutputDeviceId shouldn't be called during AddSwitcher.
- Guido Urdaneta
- 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 15 comments![]( "Open in Gerrit")
(Have not looked though authorization yet, publishing what I had time for)
// It can switch to UI thread for retrieving the main frame id for the given
// given native frame id in the call of SetPreferredSinkId(). Once it is
// retrieved, it will be cached.Implementation details, remove from the header.
// as singleton per the browser instance. While most APIs are
// called on the IO thread, certain APIs are called on the UI thread when called
// from the WebContentsObserver.
Remove: repetition
// The class is responsible for managing the audio output device for top-level
// frame as well as all the sub pages if they use system default audio output.This belongs to the interface description.
virtual void DeleteRenderFrameHostid(DeleteRenderFrameHostId
virtual void AddRenderFrameHostid(GlobalRenderFrameHostId native_frame_id,AddRenderFrameHostId
virtual void UnregisterMainFrame(RenderFrameHost* render_frame_host) = 0;Let's call it UnregisterMainFrameOnUIThread()?
virtual void SetPreferredSinkId(GlobalRenderFrameHostId native_frame_id,Could you remind me why it is called "native"?
// output device for the page whether it is main or sub frames.Could you give a better explanation re: main/sub frames?
preferred_sink_id_ = sink_id;Sunggook ChueShouldn't the setter also update the switchers if necessary?
Sunggook Chueswitcher is updated by active_device_switchers() directly because device_swithcer could be added multiple times for single 'preferred sink id' ('single PreferredSinkIdToSwitcher will have single 'preferred sink id').
Olga SharonovaPlease reactivate it if my above explanation isn't sufficient. thanks.
Sunggook ChueI've asked earlier on many occasions to not resolve the comments which are discussions. I will resolve them when I'm satisfied with the outcome of the discussion. Resolved comments, as I explain, get lost, and it's a waste of my time to recall a discussion and search for it.
I won't be reviewing at all the uploads which do not satisfy this request. Thank you for understanding.
---
I don't understand this explanation, please see the other comments.
Config has (1) preferred sink id (2) device switcher list for 'main frame id'.
The inserting of (1) and (2) comes from the different times (SetPreferredSinkId for (1), and AddSwitcher for (2).
So, there is no need to edit the (2) during SetPreferredSinkId API call.
I guess this it no applicable any longer.
// The class to store the preferred sink id for the render frame host and its
// all sub frames. For this purpose, it stores audio device switchers
// as well as preferred sink id by the main render frame host id.Sunggook ChueThis class controls preferred sink id for all switchers registered as belonging to a tree of a given main frame.
---
"it stores audio device switchers as well as preferred sink id by the main render frame host id." is written in c++ below, no need to add it to the comment.
Olga Sharonovaremoved the latter part.
Sunggook ChueCould you please fix the comment. What is store is not interesting, it's important what it is responsible for.
updated "The class to provide switchers and preferred sink id for the main frame if
the preferred sink id is set for the main frame.".please suggest if you have any others in mind?
Okay, but "if" part is not true: switchers are always registered.
I proposed the wording in the very first comment:
"This class controls preferred sink id for all switchers registered as belonging to a tree of a given main frame."
Maybe correct it if something is wrong and use it?
if (!media::AudioDeviceDescription::IsDefaultDevice(preferred_sink_id())) {We don't need to check IsDefaultDevice(). Switchers knows how to deal with the default device. Also we don't check for it in other places.
UnregisterMainFrameOnIOThread(main_frame_id);Sunggook ChueIs it correct? What if SetPreferredSinkId is called, then a switcher is added, then it's removed, then a new one is added? The information about preferred sink is lost, new switcher's device id won't be updated.
(You also need a unit test for that case.)
Olga SharonovaYou're right, we have to Unregister only when the config's sink id is 'default'. thanks.
Sunggook Chue1) Which uint tests would cover this case?
2) I don't think we need to remove it at all. It will be removed when the main frame is gone, wouldn't it?
ok,
We need a unit test, sine we had a bug which was not discovered in tests.
// Copyright 2024 The Chromium AuthorsIt looks like we need more testing, especially regarding addition/removal of entities (see my other comment). Could you please identify various sequences of events to test and add the appropriate testing?
EXPECT_CALL(*main_frame_switcher.get(),Everywhere: Please set expectations before operations
Related details
- Guido Urdaneta
- 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 15 comments![]( "Open in Gerrit")
See the other CL re: how to fix the switcher
updated, please reactivate if not resolved as you intended.
thanks
// It can switch to UI thread for retrieving the main frame id for the given
// given native frame id in the call of SetPreferredSinkId(). Once it is
// retrieved, it will be cached.Implementation details, remove from the header.
Done
// as singleton per the browser instance. While most APIs are
// called on the IO thread, certain APIs are called on the UI thread when called
// from the WebContentsObserver.
Sunggook ChueRemove: repetition
Done
// The class is responsible for managing the audio output device for top-level
// frame as well as all the sub pages if they use system default audio output.This belongs to the interface description.
Done
virtual void DeleteRenderFrameHostid(Sunggook ChueDeleteRenderFrameHostId
Done
virtual void AddRenderFrameHostid(GlobalRenderFrameHostId native_frame_id,Sunggook ChueAddRenderFrameHostId
Done
virtual void UnregisterMainFrame(RenderFrameHost* render_frame_host) = 0;Let's call it UnregisterMainFrameOnUIThread()?
Done
virtual void SetPreferredSinkId(GlobalRenderFrameHostId native_frame_id,Could you remind me why it is called "native"?
I believe it was initially commented by you and I agreed that as it clearly suggest it is the original frame id that was requested. We can rename it as 'requested_frame_id' or just 'frame_id'.
// output device for the page whether it is main or sub frames.Could you give a better explanation re: main/sub frames?
add more details, please suggest if not sufficient. thanks.
// The class to store the preferred sink id for the render frame host and its
// all sub frames. For this purpose, it stores audio device switchers
// as well as preferred sink id by the main render frame host id.Sunggook ChueThis class controls preferred sink id for all switchers registered as belonging to a tree of a given main frame.
---
"it stores audio device switchers as well as preferred sink id by the main render frame host id." is written in c++ below, no need to add it to the comment.
Olga Sharonovaremoved the latter part.
Sunggook ChueCould you please fix the comment. What is store is not interesting, it's important what it is responsible for.
Olga Sharonovaupdated "The class to provide switchers and preferred sink id for the main frame if
the preferred sink id is set for the main frame.".please suggest if you have any others in mind?
Okay, but "if" part is not true: switchers are always registered.
I proposed the wording in the very first comment:
"This class controls preferred sink id for all switchers registered as belonging to a tree of a given main frame."
Maybe correct it if something is wrong and use it?
thanks for suggestion, it would be much easier for me to suggest comment like this :).
if (!media::AudioDeviceDescription::IsDefaultDevice(preferred_sink_id())) {We don't need to check IsDefaultDevice(). Switchers knows how to deal with the default device. Also we don't check for it in other places.
Done
UnregisterMainFrameOnIOThread(main_frame_id);Sunggook ChueIs it correct? What if SetPreferredSinkId is called, then a switcher is added, then it's removed, then a new one is added? The information about preferred sink is lost, new switcher's device id won't be updated.
(You also need a unit test for that case.)
Olga SharonovaYou're right, we have to Unregister only when the config's sink id is 'default'. thanks.
Sunggook Chue1) Which uint tests would cover this case?
2) I don't think we need to remove it at all. It will be removed when the main frame is gone, wouldn't it?
Olga Sharonovaok,
We need a unit test, sine we had a bug which was not discovered in tests.
CleanupWithDeletedSwitchers, RemoveSwitcherDoesNotCallSwitchDeviceOnActiveStream, RemoveSwitcherMainFrameDoesNotCallSwitchDeviceOnActiveStream will hit this API.
// Copyright 2024 The Chromium AuthorsIt looks like we need more testing, especially regarding addition/removal of entities (see my other comment). Could you please identify various sequences of events to test and add the appropriate testing?
It already has few, but add more.
EXPECT_CALL(*main_frame_switcher.get(),Everywhere: Please set expectations before operations
- Guido Urdaneta
- 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 2 comments![]( "Open in Gerrit")
Olga Sharonovathanks
I think you forgot to upload the new PS?
virtual void SetPreferredSinkId(GlobalRenderFrameHostId native_frame_id,Sunggook ChueCould you remind me why it is called "native"?
I believe it was initially commented by you and I agreed that as it clearly suggest it is the original frame id that was requested. We can rename it as 'requested_frame_id' or just 'frame_id'.
I don't recall suggesting the word "native" specifically. It's a bit confusing in the current context. Using frame_id and main_frame_id sounds good.
Related details
- Guido Urdaneta
- 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 3 comments![]( "Open in Gerrit")
Olga Sharonovathanks
I think you forgot to upload the new PS?
Acknowledged
thanks
virtual void SetPreferredSinkId(GlobalRenderFrameHostId native_frame_id,Sunggook ChueCould you remind me why it is called "native"?
Olga SharonovaI believe it was initially commented by you and I agreed that as it clearly suggest it is the original frame id that was requested. We can rename it as 'requested_frame_id' or just 'frame_id'.
I don't recall suggesting the word "native" specifically. It's a bit confusing in the current context. Using frame_id and main_frame_id sounds good.
- Guido Urdaneta
- 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 22 comments![]( "Open in Gerrit")
// Removes the entry for the render frame host.Unregisters a subframe
// Adds the entry for the render frame host.Registers a frame as a subframe of a given main frame.
What if frame_id and main_frame_is are the same?
// Removes the entry for the render frame host.Unregisters the main frame and all its subframes with their switchers
// Gets the preferred device id for the render frame host if an entry exists.if it is registered, or an empty stream otherwise
// Gets the preferred device id for the render frame host if an entry exists.Returns
// `frame_id` is the id of the render frame host where the audio
// playback is is requested. `device_switcher` is the device switcher to bethe switcher belongs to.
(there is nothing about audio playback in this API)
// Adds the device switcher to the active device switchers list.remove "active" everywhere
// `frame_id` is the id of the render frame host. It is expected that
// it is main frame's id. If not, the API will return authorization error.
// `hashed_sink_id` is the id of the sink to be set as the preferred sink.It does not seem implementation follows this. What is the actual intent?
// any added device switcher will be notified with preferred sink id ifupdated
// any added device switcher will be notified with preferred sink id ifAll switchers belonging to frames in this main frame tree
// Sets the preferred sink id for the render frame host. At this moment,the main frame and all its subframes.
// This manager ensures that audio output can be adjusted for both
// types of frames, providing a seamless audio experience across the
// entire webpage.What is the intent of this message?
// contain separate documents or applications. These sub frames may
// require their own audio output settings, allowing for independent
// audio control within the context of the overall page.What does this mean? What do you refer to?
// Sub Frame: A secondary content area, such as an iframe, that can
// contain separate documents or applications. These sub frames mayNot needed
// in the main frame affect the primary audio stream for the user.There is no concept of "primary audio stream".
// Main Frame: The primary content area of the webpage, typically where
// the main application or document resides. Changes to the audio outputnot needed
// This abstract class manages the preferred audio output device.Please fix all the documentation for this class and use consistent terminology throughout all the comments. I added some recommendations below.
Class-level comment can be as simple as something like
This class keeps track of which frame tree each switcher belong to, and applies preferred output device changes to all switchers belonging to a tree of a given main frame.
// of its permission state.The header says it should fail if frame_id is not the main frame
UnregisterMainFrameOnIOThread(main_frame_id);Sunggook ChueIs it correct? What if SetPreferredSinkId is called, then a switcher is added, then it's removed, then a new one is added? The information about preferred sink is lost, new switcher's device id won't be updated.
(You also need a unit test for that case.)
Olga SharonovaYou're right, we have to Unregister only when the config's sink id is 'default'. thanks.
Sunggook Chue1) Which uint tests would cover this case?
2) I don't think we need to remove it at all. It will be removed when the main frame is gone, wouldn't it?
Olga Sharonovaok,
Sunggook ChueWe need a unit test, sine we had a bug which was not discovered in tests.
CleanupWithDeletedSwitchers, RemoveSwitcherDoesNotCallSwitchDeviceOnActiveStream, RemoveSwitcherMainFrameDoesNotCallSwitchDeviceOnActiveStream will hit this API.
Which test does exactly what is described above? (Oct 03 coment)
TEST_F(PreferredAudioOutputDeviceManagerImplTest, SwitchDeviceOnActiveStream) {There is no concept of active streams in this area of the code. Could you chose another name? (I'm not sure what you want to say, so don't have a proposal)
SetPrefferedSinkIdFromSubframeDoesSwitchDevice) {Could you clarify: SetPreferredSinkId() documentation says that it can only be called for the main frame. What does the test name mean and what are we testing here?
Related details
- Guido Urdaneta
- 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 24 comments![]( "Open in Gerrit")
thanks, please reactivate if resolution isn't appropriate. thanks.
// Removes the entry for the render frame host.Sunggook ChueUnregisters a subframe
Done
Registers a frame as a subframe of a given main frame.
What if frame_id and main_frame_is are the same?
If doesn't check whether it is same or not, just add them. updated comments.
Unregisters the main frame and all its subframes with their switchers
Done
// Gets the preferred device id for the render frame host if an entry exists.Sunggook ChueReturns
Done
// Gets the preferred device id for the render frame host if an entry exists.if it is registered, or an empty stream otherwise
Done
// device switchers list.Sunggook Chueframe_id is
Done
// `frame_id` is the id of the render frame host where the audio
// playback is is requested. `device_switcher` is the device switcher to bethe switcher belongs to.
(there is nothing about audio playback in this API)
Done
// Adds the device switcher to the active device switchers list.Sunggook Chueremove "active" everywhere
Done
// `frame_id` is the id of the render frame host. It is expected that
// it is main frame's id. If not, the API will return authorization error.
// `hashed_sink_id` is the id of the sink to be set as the preferred sink.It does not seem implementation follows this. What is the actual intent?
remove the line if it is confusing and doesn't add any value.
// any added device switcher will be notified with preferred sink id ifSunggook Chueupdated
Done
// any added device switcher will be notified with preferred sink id ifAll switchers belonging to frames in this main frame tree
Done
// Sets the preferred sink id for the render frame host. At this moment,the main frame and all its subframes.
Done
// This manager ensures that audio output can be adjusted for both
// types of frames, providing a seamless audio experience across the
// entire webpage.What is the intent of this message?
Done
// contain separate documents or applications. These sub frames may
// require their own audio output settings, allowing for independent
// audio control within the context of the overall page.What does this mean? What do you refer to?
Done
// Sub Frame: A secondary content area, such as an iframe, that can
// contain separate documents or applications. These sub frames maySunggook ChueNot needed
Done
// in the main frame affect the primary audio stream for the user.There is no concept of "primary audio stream".
Done
// Main Frame: The primary content area of the webpage, typically where
// the main application or document resides. Changes to the audio outputSunggook Chuenot needed
Done
// This abstract class manages the preferred audio output device.Please fix all the documentation for this class and use consistent terminology throughout all the comments. I added some recommendations below.
Class-level comment can be as simple as something like
This class keeps track of which frame tree each switcher belong to, and applies preferred output device changes to all switchers belonging to a tree of a given main frame.
Done
The header says it should fail if frame_id is not the main frame
no in header (anymore). it will be allowed as long as AudioOutputAuthorizationHandler allows it.
UnregisterMainFrameOnIOThread(main_frame_id);Sunggook ChueIs it correct? What if SetPreferredSinkId is called, then a switcher is added, then it's removed, then a new one is added? The information about preferred sink is lost, new switcher's device id won't be updated.
(You also need a unit test for that case.)
Olga SharonovaYou're right, we have to Unregister only when the config's sink id is 'default'. thanks.
Sunggook Chue1) Which uint tests would cover this case?
2) I don't think we need to remove it at all. It will be removed when the main frame is gone, wouldn't it?
Olga Sharonovaok,
Sunggook ChueWe need a unit test, sine we had a bug which was not discovered in tests.
Olga SharonovaCleanupWithDeletedSwitchers, RemoveSwitcherDoesNotCallSwitchDeviceOnActiveStream, RemoveSwitcherMainFrameDoesNotCallSwitchDeviceOnActiveStream will hit this API.
Which test does exactly what is described above? (Oct 03 coment)
Added new test that test the exact one: AddAterRemovalAfterSetId
Sunggook ChueIt looks like we need more testing, especially regarding addition/removal of entities (see my other comment). Could you please identify various sequences of events to test and add the appropriate testing?
It already has few, but add more.
Done
TEST_F(PreferredAudioOutputDeviceManagerImplTest, SwitchDeviceOnActiveStream) {There is no concept of active streams in this area of the code. Could you chose another name? (I'm not sure what you want to say, so don't have a proposal)
I tried to mean, playing one. replace it with Playing.
SetPrefferedSinkIdFromSubframeDoesSwitchDevice) {Could you clarify: SetPreferredSinkId() documentation says that it can only be called for the main frame. What does the test name mean and what are we testing here?
I think I removed that one a while ago, it will follow the pattern of the setSinkId where AudioOutputAuthorizationHandler will validate it.
Related details
- Guido Urdaneta
- 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 17 comments![]( "Open in Gerrit")
virtual void DeleteRenderFrameHostId(GlobalRenderFrameHostId frame_id) = 0;What happens with switchers?
// Registers a frame as a subframe of a given main frame.See above: either we call it "frame" everywhere, or "render frame host". (and it's quite awkward to use "render frame host", so again, I propose "frame")
// `render_frame_host` is the render frame host for which the entry is to beEither we call it "frame" everywhere, or "render frame host". (I suggest "frame" because it shorter and clear anyways, but up to you)
// `frame_id` is the id of the render frame host where the switcher belongs
// to.
virtual const std::string& GetPreferredSinkId(There is nothing about switchers here.
// added to the device. switchers list.remove
// will be updated with preferred sink id if its main frame id matches.this is essentially "belonging to the frame tree", so can be dropped, because already stated earlier
// At this moment, All switchers belonging to frames in this main frame treeif frame_id does not have to be the main frame, then
"all switchers belonging to the same frame tree as the given frame"
// This class keeps track of which frame tree each switcher belong to,nit: belongs
// `frame_id` is the id of the render frame host. It is expected that
// it is main frame's id. If not, the API will return authorization error.
// `hashed_sink_id` is the id of the sink to be set as the preferred sink.Sunggook ChueIt does not seem implementation follows this. What is the actual intent?
remove the line if it is confusing and doesn't add any value.
This is a significant change of the contract. Could you explain the decision?
// HTMLMediaElement::setSinkId. AudioOutputAuthorizationHandler will validate
// against for it.
will validate it.
AuthorizationCompleted(frame_id, hashed_sink_id, std::move(callback),We have indeterministic behavior here: if SetPreferredSinkId() is called with a non-default device, it goes to the authorization handler and jumps through some thread, but if it's called with a default device, it's processed immediately. And then we potentially jump through UI thread to get the main frame.
So it's possible that
SetPreferredSinkId(non-default); SetPreferredSinkId(default)
will result in a sequence
OnGetMainFrameId(default), OnGetMainFrameId(non-default).
I.e. the order sink switches are applied is not the same they are called in.
One way to fix it is something like this:
introduce per main frame request id counter which is incremented every time SetPreferredSinkId() is called, and pass it all the way to OnGetMainFrameId(). Then in OnGetMainFrameId() if the request_number is lower than the current counter value, then we just don't do anything. I.e. we always apply only the latest preferred sink if there was a rapid sequence of SetPreferredSinkId() calls for a given main frame tree.
// The main frame id is not cached yet.Under what circumstances is it possible?
If it's possible, then why would we have two difference mechanisms to obtain mapping? If we keep this code, why would we not just drop Add/RemoveRenderFrameHostId?
void PreferredAudioOutputDeviceManagerImpl::OnGetMainFrameId(OnMainFrameIdReceived
void PreferredAudioOutputDeviceManagerImpl::DeleteRenderFrameHostId(Add/Remove (not Delete)
TEST_F(PreferredAudioOutputDeviceManagerImplTest, SwitchDeviceOnPlaying) {Do you mean during playing? And during playing what? The tests do not actively start playing anything. OnPlaying is still confusing (everywhere). Could you formulate it in concepts of the CL?
SetPrefferedSinkIdFromSubframeDoesSwitchDevice) {Sunggook ChueCould you clarify: SetPreferredSinkId() documentation says that it can only be called for the main frame. What does the test name mean and what are we testing here?
I think I removed that one a while ago, it will follow the pattern of the setSinkId where AudioOutputAuthorizationHandler will validate it.
Acknowledged
// It adss switcher after removal that does not lose the registeredadds
Related details
- Guido Urdaneta
- 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 17 comments![]( "Open in Gerrit")
thanks
virtual void DeleteRenderFrameHostId(GlobalRenderFrameHostId frame_id) = 0;What happens with switchers?
can you clarify question? The caller removes frame_id from the cache when it won't be used anymore by any switchers.
// Registers a frame as a subframe of a given main frame.See above: either we call it "frame" everywhere, or "render frame host". (and it's quite awkward to use "render frame host", so again, I propose "frame")
Done
// `render_frame_host` is the render frame host for which the entry is to beEither we call it "frame" everywhere, or "render frame host". (I suggest "frame" because it shorter and clear anyways, but up to you)
ok, uses frame.
// `frame_id` is the id of the render frame host where the switcher belongs
// to.
virtual const std::string& GetPreferredSinkId(There is nothing about switchers here.
Done
// added to the device. switchers list.Sunggook Chueremove
Done
// will be updated with preferred sink id if its main frame id matches.this is essentially "belonging to the frame tree", so can be dropped, because already stated earlier
Done
// At this moment, All switchers belonging to frames in this main frame treeif frame_id does not have to be the main frame, then
"all switchers belonging to the same frame tree as the given frame"
Done
// This class keeps track of which frame tree each switcher belong to,Sunggook Chuenit: belongs
Done
// `frame_id` is the id of the render frame host. It is expected that
// it is main frame's id. If not, the API will return authorization error.
// `hashed_sink_id` is the id of the sink to be set as the preferred sink.Sunggook ChueIt does not seem implementation follows this. What is the actual intent?
Olga Sharonovaremove the line if it is confusing and doesn't add any value.
This is a significant change of the contract. Could you explain the decision?
It was my initial proposal to support main frame only, but it was suggested internal review to follow existing setSinkId pattern as we thought JS developer are more familiar with that API. However, it is open to change based on the partners' feedback until we will enable the feature.
// HTMLMediaElement::setSinkId. AudioOutputAuthorizationHandler will validate
// against for it.
Sunggook Chuewill validate it.
Done
AuthorizationCompleted(frame_id, hashed_sink_id, std::move(callback),We have indeterministic behavior here: if SetPreferredSinkId() is called with a non-default device, it goes to the authorization handler and jumps through some thread, but if it's called with a default device, it's processed immediately. And then we potentially jump through UI thread to get the main frame.
So it's possible that
SetPreferredSinkId(non-default); SetPreferredSinkId(default)
will result in a sequence
OnGetMainFrameId(default), OnGetMainFrameId(non-default).I.e. the order sink switches are applied is not the same they are called in.
One way to fix it is something like this:
introduce per main frame request id counter which is incremented every time SetPreferredSinkId() is called, and pass it all the way to OnGetMainFrameId(). Then in OnGetMainFrameId() if the request_number is lower than the current counter value, then we just don't do anything. I.e. we always apply only the latest preferred sink if there was a rapid sequence of SetPreferredSinkId() calls for a given main frame tree.
sounds good for your suggested algorithm.
// The main frame id is not cached yet.Under what circumstances is it possible?
If it's possible, then why would we have two difference mechanisms to obtain mapping? If we keep this code, why would we not just drop Add/RemoveRenderFrameHostId?
If SetPreferredSinkId called before any AddSwitcher, then it would hit this one.
We could drop it. However, if it hits cache, then we could avoid UI thread switching.
Please let me know what you think as initially it was your idea (register frameId/mainFrameId) to avoid UI thread switching as much as possible.
void PreferredAudioOutputDeviceManagerImpl::OnGetMainFrameId(Sunggook ChueOnMainFrameIdReceived
Done
void PreferredAudioOutputDeviceManagerImpl::DeleteRenderFrameHostId(Sunggook ChueAdd/Remove (not Delete)
Done
TEST_F(PreferredAudioOutputDeviceManagerImplTest, SwitchDeviceOnPlaying) {Do you mean during playing? And during playing what? The tests do not actively start playing anything. OnPlaying is still confusing (everywhere). Could you formulate it in concepts of the CL?
I'm not sure which name is appropriate for this. Initially ActiveStream seems right wording as it is after CreateStream is called.
How about removing this word, it seems clearer.
// It adss switcher after removal that does not lose the registeredSunggook Chueadds
Done
Related details
- Guido Urdaneta
- 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 5 comments![]( "Open in Gerrit")
// non-default and default are called back to back.Not only that, there is also a jump via UI thread sometimes.
"To serialize SetSinkId calls when them jump through different threads." would be a sufficient comment.
virtual void DeleteRenderFrameHostId(GlobalRenderFrameHostId frame_id) = 0;Sunggook ChueWhat happens with switchers?
can you clarify question? The caller removes frame_id from the cache when it won't be used anymore by any switchers.
So it's expected that if any switchers were added earlier, they are removed by now, right? If so - please add that to the comment, and make sure we have a corresponding CHECK in the implementation.
// `frame_id` is the id of the render frame host. It is expected that
// it is main frame's id. If not, the API will return authorization error.
// `hashed_sink_id` is the id of the sink to be set as the preferred sink.Sunggook ChueIt does not seem implementation follows this. What is the actual intent?
Olga Sharonovaremove the line if it is confusing and doesn't add any value.
Sunggook ChueThis is a significant change of the contract. Could you explain the decision?
It was my initial proposal to support main frame only, but it was suggested internal review to follow existing setSinkId pattern as we thought JS developer are more familiar with that API. However, it is open to change based on the partners' feedback until we will enable the feature.
Ok, thanks for clarification.
AuthorizationCompleted(frame_id, hashed_sink_id, std::move(callback),Sunggook ChueWe have indeterministic behavior here: if SetPreferredSinkId() is called with a non-default device, it goes to the authorization handler and jumps through some thread, but if it's called with a default device, it's processed immediately. And then we potentially jump through UI thread to get the main frame.
So it's possible that
SetPreferredSinkId(non-default); SetPreferredSinkId(default)
will result in a sequence
OnGetMainFrameId(default), OnGetMainFrameId(non-default).I.e. the order sink switches are applied is not the same they are called in.
One way to fix it is something like this:
introduce per main frame request id counter which is incremented every time SetPreferredSinkId() is called, and pass it all the way to OnGetMainFrameId(). Then in OnGetMainFrameId() if the request_number is lower than the current counter value, then we just don't do anything. I.e. we always apply only the latest preferred sink if there was a rapid sequence of SetPreferredSinkId() calls for a given main frame tree.
sounds good for your suggested algorithm.
The caveat is the counter needs to be per main frame - see my comment above. Otherwise we may be dropping requests from a different frame tree.
Hmmm.. So we'll have to reverse the order: first look up the main frame and update its request id, then do the authorization. Will this work?
We can make request_id a part of MainFramePreferredSinkIdConfig, add GetNextRequestId() and have SetPreferredSinkId(..., request_id) there which would do nothing if the request has expired. Does it make sense?
// The main frame id is not cached yet.Sunggook ChueUnder what circumstances is it possible?
If it's possible, then why would we have two difference mechanisms to obtain mapping? If we keep this code, why would we not just drop Add/RemoveRenderFrameHostId?
If SetPreferredSinkId called before any AddSwitcher, then it would hit this one.
We could drop it. However, if it hits cache, then we could avoid UI thread switching.
Please let me know what you think as initially it was your idea (register frameId/mainFrameId) to avoid UI thread switching as much as possible.
Again, the way caching is done was your proposal, mine was to cache it in the switcher :)
Ok, I see what you mean. But it's not if SetPreferredSinkId is called before any AddSwitcher, but rather if SetPreferredSinkId is called before the frame accessed RendererAudioOutputStreamFactory for the first time - that's when we call AddRenderFrameHostId - right?
If so - could you document it? Something like "SetSinkId() is called before AddRenderFrameHostId(), i.e. the frame called SetSinkId() before creating any audio outputs"
Related details
- Guido Urdaneta
- 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 5 comments![]( "Open in Gerrit")
thanks
Not only that, there is also a jump via UI thread sometimes.
"To serialize SetSinkId calls when them jump through different threads." would be a sufficient comment.
removed request_id_
virtual void DeleteRenderFrameHostId(GlobalRenderFrameHostId frame_id) = 0;Sunggook ChueWhat happens with switchers?
Olga Sharonovacan you clarify question? The caller removes frame_id from the cache when it won't be used anymore by any switchers.
So it's expected that if any switchers were added earlier, they are removed by now, right? If so - please add that to the comment, and make sure we have a corresponding CHECK in the implementation.
yes, we already have this.
DCHECK(render_frame_host_id_to_main_frame_id_.find(frame_id) !=
render_frame_host_id_to_main_frame_id_.end());
move to CHECK.
AuthorizationCompleted(frame_id, hashed_sink_id, std::move(callback),Sunggook ChueWe have indeterministic behavior here: if SetPreferredSinkId() is called with a non-default device, it goes to the authorization handler and jumps through some thread, but if it's called with a default device, it's processed immediately. And then we potentially jump through UI thread to get the main frame.
So it's possible that
SetPreferredSinkId(non-default); SetPreferredSinkId(default)
will result in a sequence
OnGetMainFrameId(default), OnGetMainFrameId(non-default).I.e. the order sink switches are applied is not the same they are called in.
One way to fix it is something like this:
introduce per main frame request id counter which is incremented every time SetPreferredSinkId() is called, and pass it all the way to OnGetMainFrameId(). Then in OnGetMainFrameId() if the request_number is lower than the current counter value, then we just don't do anything. I.e. we always apply only the latest preferred sink if there was a rapid sequence of SetPreferredSinkId() calls for a given main frame tree.
Olga Sharonovasounds good for your suggested algorithm.
The caveat is the counter needs to be per main frame - see my comment above. Otherwise we may be dropping requests from a different frame tree.
Hmmm.. So we'll have to reverse the order: first look up the main frame and update its request id, then do the authorization. Will this work?
We can make request_id a part of MainFramePreferredSinkIdConfig, add GetNextRequestId() and have SetPreferredSinkId(..., request_id) there which would do nothing if the request has expired. Does it make sense?
We don't create MainFramePreferredSinkIdConfig when it failed on the Authorization.
so, authorization check needs to made as early as possible.
Â
How about calling RequestDeviceAuthorization even with default device id, which seems much cleaner? (I will update the code with this scheme as the previous request_id_ doesn't work out).
Please let me know if you still prefer your suggestion?
(I think the chance of the initial concern might not occurs in real time, we might overengineer here :).
// The main frame id is not cached yet.Sunggook ChueUnder what circumstances is it possible?
If it's possible, then why would we have two difference mechanisms to obtain mapping? If we keep this code, why would we not just drop Add/RemoveRenderFrameHostId?
Olga SharonovaIf SetPreferredSinkId called before any AddSwitcher, then it would hit this one.
We could drop it. However, if it hits cache, then we could avoid UI thread switching.
Please let me know what you think as initially it was your idea (register frameId/mainFrameId) to avoid UI thread switching as much as possible.
Again, the way caching is done was your proposal, mine was to cache it in the switcher :)
Ok, I see what you mean. But it's not if SetPreferredSinkId is called before any AddSwitcher, but rather if SetPreferredSinkId is called before the frame accessed RendererAudioOutputStreamFactory for the first time - that's when we call AddRenderFrameHostId - right?
If so - could you document it? Something like "SetSinkId() is called before AddRenderFrameHostId(), i.e. the frame called SetSinkId() before creating any audio outputs"
- Guido Urdaneta
- 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 2 comments![]( "Open in Gerrit")
AuthorizationCompleted(frame_id, hashed_sink_id, std::move(callback),Sunggook ChueWe have indeterministic behavior here: if SetPreferredSinkId() is called with a non-default device, it goes to the authorization handler and jumps through some thread, but if it's called with a default device, it's processed immediately. And then we potentially jump through UI thread to get the main frame.
So it's possible that
SetPreferredSinkId(non-default); SetPreferredSinkId(default)
will result in a sequence
OnGetMainFrameId(default), OnGetMainFrameId(non-default).I.e. the order sink switches are applied is not the same they are called in.
One way to fix it is something like this:
introduce per main frame request id counter which is incremented every time SetPreferredSinkId() is called, and pass it all the way to OnGetMainFrameId(). Then in OnGetMainFrameId() if the request_number is lower than the current counter value, then we just don't do anything. I.e. we always apply only the latest preferred sink if there was a rapid sequence of SetPreferredSinkId() calls for a given main frame tree.
Olga Sharonovasounds good for your suggested algorithm.
Sunggook ChueThe caveat is the counter needs to be per main frame - see my comment above. Otherwise we may be dropping requests from a different frame tree.
Hmmm.. So we'll have to reverse the order: first look up the main frame and update its request id, then do the authorization. Will this work?
We can make request_id a part of MainFramePreferredSinkIdConfig, add GetNextRequestId() and have SetPreferredSinkId(..., request_id) there which would do nothing if the request has expired. Does it make sense?
We don't create MainFramePreferredSinkIdConfig when it failed on the Authorization.
so, authorization check needs to made as early as possible.
Â
How about calling RequestDeviceAuthorization even with default device id, which seems much cleaner? (I will update the code with this scheme as the previous request_id_ doesn't work out).Please let me know if you still prefer your suggestion?
(I think the chance of the initial concern might not occurs in real time, we might overengineer here :).
We don't create MainFramePreferredSinkIdConfig when it failed on the Authorization.
So we can delete it if authorization failed and there are no requests in progress, for example?
How about calling RequestDeviceAuthorization even with default device id, which seems much cleaner?
We should do it, yes, because all authorization decisions need to be done in one place, so please keep the change.
But I don't think it helps with our specific problem: AudioOutputAuthorizationHandler does not serialize its replies - take a look at its implementation.
I agree this may be a rare case. But I prefer to invest time now in making it robust, rather than a year later spend two weeks debugging some obscure edge case which all of a sudden is very popular.
TEST_F(PreferredAudioOutputDeviceManagerImplTest, SwitchDeviceOnPlaying) {Sunggook ChueDo you mean during playing? And during playing what? The tests do not actively start playing anything. OnPlaying is still confusing (everywhere). Could you formulate it in concepts of the CL?
I'm not sure which name is appropriate for this. Initially ActiveStream seems right wording as it is after CreateStream is called.
How about removing this word, it seems clearer.
What condition did you want to describe with these words? Could you elaborate?
Related details
- Guido Urdaneta
- 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. |
Olga Sharonova (Gerrit)
Olga Sharonova added 1 comment![]( "Open in Gerrit")
BTW: if a follow-up device authorization has not succeeded we don't remove MainFramePreferredSinkIdConfig which has already been created.
And we do add MainFramePreferredSinkIdConfig entries for switchers without any authorization.
What invariant do you want to maintain around this? Maybe it just does not matter that we cache it before authorization?