Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8496007:
A patch making the pulseaudio working with the threaded mainloop. (Closed)
|
Created:
9 years, 1 month ago by no longer working on chromium Modified:
7 years, 10 months ago CC:
chromium-reviews, hclam+watch_chromium.org, ddorwin+watch_chromium.org, fischman+watch_chromium.org, acolwell+watch_chromium.org, annacc+watch_chromium.org, ajwong+watch_chromium.org, ihf+watch_chromium.org, Chris Rogers, henrika (OOO until Aug 14) Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionA patch making the pulseaudio working with the glib_mainloop, it is a callback driven implementation,
Patch Set 1 : '' #
Total comments: 35
Patch Set 2 : '' #
Total comments: 3
Patch Set 3 : '' #Patch Set 4 : rebase #Messages
Total messages: 14 (0 generated)
Please review the new native pulseaudio implementation. This patch should make the pulseaudio IO handling in par with ALSA IO handling. Thanks, BR, /SX
I know next to nothing about pulseaudio unfortunately, so I'll leave those details to other reviewers. http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.cc File media/audio/linux/pulse_output.cc (right): http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:15: // TODO(xians): Do we support sample format rather than PA_SAMPLE_S16LE? which sample format? http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:50: static_cast<PulseAudioOutputStream*>(p_this); this should be reinterpret_cast. static_cast should only be used when the types are related. http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:57: default: default label should be last. http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:72: static_cast<PulseAudioOutputStream*>(p_this); reinterpret_cast http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:119: pa_glib_mainloop_ = pa_glib_mainloop_new(NULL); first DCHECK that pa_glib_mainloop_ is NULL http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:120: DCHECK(pa_glib_mainloop_); this and the DLOG below cover the same case. You can change this DCHECK to: DCHECK(pa_glib_mainloop_) << "Open: failed to create PA glib mainloop"; and remove the DLOG below http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:131: DLOG(ERROR) << "Open: failed to create PA context"; DCHECK? http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:148: base::PlatformThread::Sleep(2); Is using Sleep the only option? I'll leave this to the other reviewers but you no doubt know that Sleep always raises suspicion :) http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:193: CHECK(callback); did you mean DCHECK? CHECK also applies to release and will kill the process if someone passes NULL to this function. However, it looks like |callback| is only used if source_callback_ hasn't already been set, so it looks like passing NULL for callback_ is actually a supported case. Also, can you move such checks (checking validity of arguments) to the top of the function? Otherwise we might miss bugs where e.g. someone passes NULL to this function but stream_stopped_ is false for some reason. http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:204: pa_buffer_attributes.maxlength = (uint32_t) -1; static_cast (never use C style cast) http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:212: (pa_stream_flags_t)0, static_cast http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:221: // Flush the stream. should we [D]CHECK here that source_callback_ == callback? http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:297: scoped_refptr<media::DataBuffer> packet = nit: prefer constructor syntax for types that have a constructor http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:305: if (buffer_->forward_bytes() < (unsigned int)bytes_to_fill) { static_cast http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:338: if (buffer_size < (unsigned int)bytes_to_fill) cast http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.h File media/audio/linux/pulse_output.h (right): http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.h:55: static void ContextStateCallback(pa_context* context, void* p_this); p_this -> this_ptr Actually, the documentation for this callback says that the name of the last argument is |user_data|. Can we just use that? http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.h:57: void* p_this); same here
Just to be safe: did you test it on Chromebook? http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.cc File media/audio/linux/pulse_output.cc (right): http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:148: base::PlatformThread::Sleep(2); On 2011/11/08 10:51:36, tommi wrote: > Is using Sleep the only option? I'll leave this to the other reviewers but you > no doubt know that Sleep always raises suspicion :) Sleep() may be not the best, but definitely simplest and most reliable solution. I recently added proper polling for first data packet to become ready when starting audio stream, incorporating it into message loop. It was not easy and caused lot of crashes. I hope we fix all of them, but unfortunately I would not bet on it. I'd recommend to add some timeout. I.e. fail if state does not change in 100ms (50ms? 200ms?).
On Tue, Nov 8, 2011 at 6:22 PM, <enal@google.com> wrote: > Just to be safe: did you test it on Chromebook? > > Hi Eugene, Not yet. Chromebook does not have pulse server running at the moment, so we can not test it at the moment. But we do have plan to test this pulse implementation on chromebook, and this will happen after it works perfectly on the browser.
On 2011/11/08 19:51:54, xians1 wrote: > On Tue, Nov 8, 2011 at 6:22 PM, <mailto:enal@google.com> wrote: > > > Just to be safe: did you test it on Chromebook? > > > > Hi Eugene, > Not yet. Chromebook does not have pulse server running at the moment, so we > can not test it at the moment. > But we do have plan to test this pulse implementation on chromebook, and > this will happen > after it works perfectly on the browser. I'm going to remove myself as a reviewer and defer to vrk@ as she's looked at more pulse stuff than I have
Changed the code based on the comments from Tommi and Eugene. Please take the second round. Thanks, /SX http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.cc File media/audio/linux/pulse_output.cc (right): http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:15: // TODO(xians): Do we support sample format rather than PA_SAMPLE_S16LE? On 2011/11/08 10:51:36, tommi wrote: > which sample format? Done. http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:50: static_cast<PulseAudioOutputStream*>(p_this); On 2011/11/08 10:51:36, tommi wrote: > this should be reinterpret_cast. > static_cast should only be used when the types are related. Done. http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:57: default: On 2011/11/08 10:51:36, tommi wrote: > default label should be last. Done. http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:72: static_cast<PulseAudioOutputStream*>(p_this); On 2011/11/08 10:51:36, tommi wrote: > reinterpret_cast Done. http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:119: pa_glib_mainloop_ = pa_glib_mainloop_new(NULL); On 2011/11/08 10:51:36, tommi wrote: > first DCHECK that pa_glib_mainloop_ is NULL Done. http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:120: DCHECK(pa_glib_mainloop_); On 2011/11/08 10:51:36, tommi wrote: > this and the DLOG below cover the same case. You can change this DCHECK to: > DCHECK(pa_glib_mainloop_) << "Open: failed to create PA glib mainloop"; > and remove the DLOG below Done. http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:148: base::PlatformThread::Sleep(2); On 2011/11/08 10:51:36, tommi wrote: > Is using Sleep the only option? I'll leave this to the other reviewers but you > no doubt know that Sleep always raises suspicion :) Use a WaitableEvent, hope it is fine. http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:148: base::PlatformThread::Sleep(2); On 2011/11/08 17:22:06, enal1 wrote: > On 2011/11/08 10:51:36, tommi wrote: > > Is using Sleep the only option? I'll leave this to the other reviewers but > you > > no doubt know that Sleep always raises suspicion :) > > Sleep() may be not the best, but definitely simplest and most reliable solution. > I recently added proper polling for first data packet to become ready when > starting audio stream, incorporating it into message loop. It was not easy and > caused lot of crashes. I hope we fix all of them, but unfortunately I would not > bet on it. > > I'd recommend to add some timeout. I.e. fail if state does not change in 100ms > (50ms? 200ms?). Use a WaitableEvent with a timeout for 200ms, hope it is fine. http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:193: CHECK(callback); On 2011/11/08 10:51:36, tommi wrote: > did you mean DCHECK? CHECK also applies to release and will kill the process if > someone passes NULL to this function. > However, it looks like |callback| is only used if source_callback_ hasn't > already been set, so it looks like passing NULL for callback_ is actually a > supported case. > It should be CHECK(), since it is not designed to handle a callback which is NULL. > Also, can you move such checks (checking validity of arguments) to the top of > the function? Otherwise we might miss bugs where e.g. someone passes NULL to > this function but stream_stopped_ is false for some reason. Good point, done. http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:204: pa_buffer_attributes.maxlength = (uint32_t) -1; On 2011/11/08 10:51:36, tommi wrote: > static_cast > (never use C style cast) Done. http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:212: (pa_stream_flags_t)0, On 2011/11/08 10:51:36, tommi wrote: > static_cast Done. http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:221: // Flush the stream. On 2011/11/08 10:51:36, tommi wrote: > should we [D]CHECK here that source_callback_ == callback? I moved the source_callback_ = callback; out of the if else section. http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:297: scoped_refptr<media::DataBuffer> packet = On 2011/11/08 10:51:36, tommi wrote: > nit: prefer constructor syntax for types that have a constructor Done. http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:305: if (buffer_->forward_bytes() < (unsigned int)bytes_to_fill) { On 2011/11/08 10:51:36, tommi wrote: > static_cast Done. http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.cc:338: if (buffer_size < (unsigned int)bytes_to_fill) On 2011/11/08 10:51:36, tommi wrote: > cast Done. http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.h File media/audio/linux/pulse_output.h (right): http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.h:55: static void ContextStateCallback(pa_context* context, void* p_this); On 2011/11/08 10:51:36, tommi wrote: > p_this -> this_ptr > > Actually, the documentation for this callback says that the name of the last > argument is |user_data|. Can we just use that? Done. http://codereview.chromium.org/8496007/diff/6/media/audio/linux/pulse_output.... media/audio/linux/pulse_output.h:57: void* p_this); On 2011/11/08 10:51:36, tommi wrote: > same here Done.
lgtm http://codereview.chromium.org/8496007/diff/8001/media/audio/linux/pulse_outp... File media/audio/linux/pulse_output.cc (right): http://codereview.chromium.org/8496007/diff/8001/media/audio/linux/pulse_outp... media/audio/linux/pulse_output.cc:16: // TODO(xians): Do we support any sample format rather than PA_SAMPLE_S16LE? rather -> other http://codereview.chromium.org/8496007/diff/8001/media/audio/linux/pulse_outp... media/audio/linux/pulse_output.cc:214: ); just keep ); on the preceding line
Can you please run Ami's tests on your new code: http://codereview.chromium.org/8418031/ (Tests are not checked in yet because there are problem on Mac, I suspect it bug in Mac OS, trying to talk with Apple guys). Run that test lot of times, e.g. browser_tests --gtest_filter=MediaBrowserTest.SeekJumper* --gtest_repeat=100 http://codereview.chromium.org/8496007/diff/8001/media/audio/linux/pulse_outp... File media/audio/linux/pulse_output.cc (right): http://codereview.chromium.org/8496007/diff/8001/media/audio/linux/pulse_outp... media/audio/linux/pulse_output.cc:23: // Also 8-bits: PA_SAMPLE_ALAW and PA_SAMPLE_ULAW I used to having comments before the related code...
Before I do a line-by-line review, I wanted to get clarity on one major concern: You are using pa_glib_mainloop_new, so where is the corresponding glib event loop? When you use the PA glib API, that means the PA commands will run on the glib event loop. I don't see any calls to g_main_context_iteration, nor do I see a MessageLoop that is wrapping glib's event loop, so I'm not sure how this works. It seems to me the only way this would work is if you're hijacking some already-existing glib event loop in Chrome that was created for an entirely different purpose... in fact, it looks like it's the UI thread for Gtk. If my understanding above is correct (and please correct me if I'm wrong!), this is not committable: we very probably don't want PA stuff on the UI thread, we don't want to interface with a MessageLoop-wrapped glib event loop without posting tasks, and definitely don't want to be doing this all inadvertently.
Hi Eugene and Victoria, Sorry that I do not have time to take a look it today, but will work on it again soon after getting some slots. Victoria, As I have already addressed before, I did not know much about the glib mainloop, and was told that I can use the existing chromium glib mainloop infrastructure, and no need to implement my own mainloop for pulseaudio. So I was supposing when I created a glib mainloop by pa_glib_mainloop_new, it will automatically map to the existing event loop that forwards the event to the PA callbacks. Maybe I misunderstand something here, since I never put any effort on really understanding it. Are you saying that I am supposed to write a event loop wrapper to handle the PA events? Any document about the glib mainloop or sample code I should look at? Thanks a lot, BR, -SX On Fri, Nov 11, 2011 at 8:32 PM, <vrk@chromium.org> wrote: > Before I do a line-by-line review, I wanted to get clarity on one major > concern: > You are using pa_glib_mainloop_new, so where is the corresponding glib > event > loop? > > When you use the PA glib API, that means the PA commands will run on the > glib > event loop. I don't see any calls to g_main_context_iteration, nor do I > see a > MessageLoop that is wrapping glib's event loop, so I'm not sure how this > works. > It seems to me the only way this would work is if you're hijacking some > already-existing glib event loop in Chrome that was created for an entirely > different purpose... in fact, it looks like it's the UI thread for Gtk. > > If my understanding above is correct (and please correct me if I'm > wrong!), this > is not committable: we very probably don't want PA stuff on the UI thread, > we > don't want to interface with a MessageLoop-wrapped glib event loop without > posting tasks, and definitely don't want to be doing this all > inadvertently. > > http://codereview.chromium.**org/8496007/<http://codereview.chromium.org/8496... >
On 2011/11/14 18:27:13, xians1 wrote: > Hi Eugene and Victoria, > > Sorry that I do not have time to take a look it today, but will work on it > again soon after getting some slots. > > Victoria, > As I have already addressed before, I did not know much about the glib > mainloop, and was told that I can use > the existing chromium glib mainloop infrastructure, and no need to > implement my own mainloop for pulseaudio. > So I was supposing when I created a glib mainloop by pa_glib_mainloop_new, > it will automatically map to the existing > event loop that forwards the event to the PA callbacks. Maybe I > misunderstand something here, since I never put any effort > on really understanding it. Ah! Yes, I think there was a misunderstanding -- unfortunately there are a lot of words that seem like synonyms but actually have distinct meanings (main loop vs MessageLoop, for instance), so it can be tricky to explain clearly... Let me try to explain it again. There are two key points: - PulseAudio events run on a main loop. You will need to create a new PulseAudio thread that is dedicated to running this main loop. - Threads in Chromium use MessageLoop. *Ideally*, your PulseAudio thread would also use a MessageLoop so that the rest of Chromium can understand it and interact with it. So the question becomes, how we implement PulseAudio's main loop using MessageLoop? One option is to implement a new pa_mainloop_api on top of Chrome's MessageLoop. But that's really hard. An alternative option that I suggested to you is to use PulseAudio's glib main loop, which will run events on a glib event loop. The advantage to doing this is that there is already an implementation for glib (MessagePumpGlib), so for the PulseAudio thread, you can use a MessageLoop created using this MessagePumpGlib as a backend and you don't have to write your own. But you still have to create a new thread, not hijack whatever existing thread is already in Chrome. Does that make more sense? Let me also say that no one is 100% confident in this suggestion :) There are no PulseAudio experts here (I've barely touched PA myself even). It's likely you will run into unforeseen pitfalls, subtle bugs, etc., and you may even need to implement this using an entirely different approach. This also means it'll be very difficult to catch bugs via code review. For these reasons, please try to get a deep understanding of whatever you end up writing. > Any document about the glib > mainloop or sample code I should look at? Unfortunately, not really. You can look at the PulseAudio documentation: http://www.freedesktop.org/software/pulseaudio/doxygen/glib-mainloop_8h.html Though many questions are better answered by looking at the PulseAudio source code: http://cgit.freedesktop.org/pulseaudio/pulseaudio/tree/src/pulse Hope that helps, and let me know if you want to discuss further or chat about other options!
On Tue, Nov 15, 2011 at 10:21 AM, <vrk@chromium.org> wrote: > On 2011/11/14 18:27:13, xians1 wrote: > >> Hi Eugene and Victoria, >> > > Sorry that I do not have time to take a look it today, but will work on it >> again soon after getting some slots. >> > > Victoria, >> As I have already addressed before, I did not know much about the glib >> mainloop, and was told that I can use >> the existing chromium glib mainloop infrastructure, and no need to >> implement my own mainloop for pulseaudio. >> So I was supposing when I created a glib mainloop by pa_glib_mainloop_new, >> it will automatically map to the existing >> event loop that forwards the event to the PA callbacks. Maybe I >> misunderstand something here, since I never put any effort >> on really understanding it. >> > > Ah! Yes, I think there was a misunderstanding -- unfortunately there are a > lot > of words that seem like synonyms but actually have distinct meanings (main > loop > vs MessageLoop, for instance), so it can be tricky to explain clearly... > > Let me try to explain it again. There are two key points: > - PulseAudio events run on a main loop. You will need to create a new > PulseAudio > thread that is dedicated to running this main loop. > - Threads in Chromium use MessageLoop. *Ideally*, your PulseAudio thread > would > also use a MessageLoop so that the rest of Chromium can understand it and > interact with it. > > So the question becomes, how we implement PulseAudio's main loop using > MessageLoop? One option is to implement a new pa_mainloop_api on top of > Chrome's > MessageLoop. But that's really hard. > > An alternative option that I suggested to you is to use PulseAudio's glib > main > loop, which will run events on a glib event loop. The advantage to doing > this is > that there is already an implementation for glib (MessagePumpGlib), so for > the > PulseAudio thread, you can use a MessageLoop created using this > MessagePumpGlib > as a backend and you don't have to write your own. But you still have to > create > a new thread, not hijack whatever existing thread is already in Chrome. Does that make more sense? > Now I understand it a bit more. What you are saying is that we should create a message_loop using MessagePumpGlib, and pass the GMainContext to PA, instead of using the default GMainContext which is usually used by the GTK. I took a quick look at the MessagePumpGlib, it seems it is specific implementation of message_pump_gtk/message_pump_x, which are for the UI. So it may mean that I need to modify this MessagePumpGlib, right? Or is there any easier way to retrieve GMainContext from the current base::Thread? > Let me also say that no one is 100% confident in this suggestion :) There > are no > PulseAudio experts here (I've barely touched PA myself even). It's likely > you > will run into unforeseen pitfalls, subtle bugs, etc., and you may even > need to > implement this using an entirely different approach. This also means it'll > be > very difficult to catch bugs via code review. For these reasons, please > try to > get a deep understanding of whatever you end up writing. > > My current understanding is: glib mainloop: It requires MessagePumpGlib (or something else?) . The best is that we can use MessagePumpGlib as the audio_thread_, and re-use it. I believe this idea may give us some trouble. glib threaded mainloop: It creates a new thread for the callbacks. But this good thing is that we know it will work, no pain after the PA implementation is done. BR, /SX
Hi all, Thanks for Victoria, it has been quite clear on what is missing on using the glib mainloop for pulseaudio. We need to add support for MessagePumpGlib to use non-default GMainContext, and use this MessagePumpGlib to provide its GMainContext to pulseaudio. I have a TODO list here: #1 Modify MessagePumpGlib to support non-default MainContext. #2 Use MessagePumpGlib for the audio_thread_, and make sure it runs as the same well as how MessagePumpLibevent does in all platform. #3 Make pulse work with the new audio_thead_. If we are lucky, it should be pretty easy, but Apparently, it is not a single day of work. But this is not what I understood before, I was told that the chromium glib had been ready for pulse, and it is the way to shoot because it is believe to be the easiest. Obviously it is not entirely true here. As Victoria has pointed out, there might some unforeseen problems for the glib, so I do not see any reason why we have to use glib. Especially it only took me 2 hours to implement a threaded mainloop solution which works really well. (given that all implementations in windows and mac are all running callbacks on a separate thread, I hope no one argues that this threaded mainloop solution requires one more thread than the glib solution). Lets come back to the purpose of this CL, we would like a native pulseaudio implementation which is in par with ALSA, and we can compare the performance. So anyway, we would like to test performance of this threaded mainloop solution in chromium. I am not saying that we will completely drop the glib solution, we may come back if we feel like further improving the performance, like reducing one more thread. But lets take it later until we are sure we should put more resource on pulse. Victoria and Eugene, any idea on the changing? If you guys agree, can you start reviewing? I just uploaded a new version which uses the threaded mainloop. Eugene, I will runt he seeker_jumper test after getting feedback.
Any update on this thread? BR, /SX |
