SFML community forums

Help => Audio => Topic started by: Foaly on September 19, 2013, 11:35:45 am

Title: Help with the design for selecting audio capture device
Post by: Foaly on September 19, 2013, 11:35:45 am
Hello everybody!
In an effort to make the requested community driven development happen (on a small scale ;)) and also extend SFMLs multimedia functionality (but also for me personally to learn what I could have done better) I would like to invite everybody to start a discussion here with the goal to find a good design for the selection of the audio capture device (https://github.com/SFML/SFML/issues/220).
Yesterday I proposed a "design" on GitHub (https://github.com/SFML/SFML/pull/470). It was more or less the code I was using and now I would like to make sure that the design holds up to the standarts of SFML. So I would like to welcome everybody to rip it apart and put it back together in a better way!
Instead of endless commits changing little things and littering the pull request, I would like to find a solid design here and then hand it over to Laurent to review it.
Laurent already mentioned in the pull request that maybe the selection of the audio capture device should be something that is only possible on initialization. I only agree partially. This is something that you should also be able to set in the initilization step. But I don't see a reason why you shouldn't be able to change it while recording. I tested it with two mics and there is no noticable gap in the samples.
But like I said I would like to hear everybody's input on this.
So please everybody come and help.
And now fire away!
Title: Re: Help with the design for selecting audio capture device
Post by: Laurent on September 19, 2013, 11:52:43 am
First, thanks for your efforts. SFML 2.2 is not progressing as fast as I wish, so I really appreciate active help from users :)

If you keep the ability to change the audio device at any time, you should call start() and stop() inside of setAudioCaptureDevice. There are indeed two problems with your current code: it is less maintainable because code is duplicated, and more importantly, onStop() is called but not onStart(). In this case both should be called for consistency, or none (this is to be decided).

Anther minor comment: you should remove "Audio" from the function names. I'd say you can even remove "Capture", since we are in the scope of sf::SoundRecorder.

And don't forget to add a space between an "if" and its opening "(" :P
Title: Re: Help with the design for selecting audio capture device
Post by: Mario on September 19, 2013, 04:06:17 pm
Think this should also include some kind of error handling, e.g. right now it seems like you'll get an odd or broken state if switching the device fails (i.e. revert back to default device?).

Also think this should at the same time cause the same functionality to be added to audio playback in some way as well. Just not sure how that should be grouped/associated (global? passed to sf::Sound's constructor with reference counting?).
Title: Re: Help with the design for selecting audio capture device
Post by: Laurent on September 19, 2013, 04:14:28 pm
Quote
Think this should also include some kind of error handling, e.g. right now it seems like you'll get an odd or broken state if switching the device fails (i.e. revert back to default device?).
Are you sure? The code should output an error message after the failure, and then reject any attempt to use the recording device. So this is perfectly clean.

Quote
Also think this should at the same time cause the same functionality to be added to audio playback in some way as well. Just not sure how that should be grouped/associated (global? passed to sf::Sound's constructor with reference counting?).
The playback device could be handled the same way at the OpenAL level, but the design has to be totally different since it impacts almost all the SFML audio classes. So I agree that the same must be done for playback, but for now let's focus on the recording device.
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on September 20, 2013, 12:30:58 pm
First of all thanks for the feedback! I am glad to see that the code is getting some attention!
Quote
Think this should also include some kind of error handling[...]
Like Laurent said the code outputs an error message and does not start recording if you pass a wrong name to the setAudioCaptureDevice method. But while testing wrong names I noticed a "bug" that confused me and made me think it's left in a broken state. I used sf::SoundBufferRecorder for testing and if you try to save a completely empty buffer you get an error message saying "Format not recognised" which is confusing and not very discribtiv in this case. But this has nothing to do with sf::SoundRecorder it's an error in sf::SoundBuffer. This should be discussed in a seperate thread.
As of for this class, it should handle wrong input in a clean way.

Quote
[...]you should call start() and stop() inside of setAudioCaptureDevice[...]
I decided not to call start() and stop(), because it makes it impossible to change of the device while recording. In start() m_samples.clear(); is called and the call to onStart() does the same thing to derived classes (for example in sf::SoundBufferRecorder onStart() clears the buffer too). The way I see it setAudioCaptureDevice() doesn't stop and (re)start the recording. It simply changes the device. The user shouldn't worry about losing samples, because he changed the device.

Before you pointed it out I didn't even noticed that onStop() is called! Of course this is a bug. But now that I look at it I am asking myself why is onStop() being called in the record() method anyway? It should be called in stop() after m_thread.wait();. That would also fix this problem.

I know that this means a little code duplication, but as I explained the code works a little different and I think the functionality overweights.

You are definitly right about the method names. They are way to long! I'm terrible at finding names... I'll change that later.
Title: Re: Help with the design for selecting audio capture device
Post by: Laurent on September 20, 2013, 01:51:53 pm
onStop() is called in the recording thread so that it's the same thread as onProcessSamples(). But I guess it won't hurt if it's called in the caller thread instead, being consistent with onStart().
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on September 20, 2013, 09:39:32 pm
OK I made a few changes. I added the possiblity to pass the name of the capture device to the start() method. I also renamed the methods and changed the caller of the onStop() thread.

Edit: i just noticed a little error in the documentation of start(), but I'll fix that tomorrow.
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on September 21, 2013, 07:39:59 pm
Alright I updated the documetation.
Are there any other suggestions?
Title: Re: Help with the design for selecting audio capture device
Post by: Laurent on September 21, 2013, 09:37:21 pm
My suggestions:

1. You must choose: it's either "device" or "device name"; don't mix both in function and variable names (I'd suggest "device" -- if one day we change what identifies a device, the function names won't have to be adjusted).

2. You can set the device with a dedicated function, or with the start() function. I don't like providing two ways of doing the same thing, it's confusing. Again, you should choose one or the other. The behaviour of getCurrentDevice() must be clarified too, according to what you'll keep for changing the device. In the end, the choice is between a persistent device that you set once, or a temporary one that you set everytime a capture starts.

3. If setDevice is called in the middle of a capture, and it fails, onStop() will never be called.
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on September 23, 2013, 01:01:27 am
Thanks for your input!
I almost agree with everything you said. Here is what I'm thinking:

1. I partially agree. The function names are too long. I think "device" should be used. From the outside (for the user) there is no difference between "device" and "deviceName". But I think for variable names deviceName should be used instead of device, so it won't be confused with captureDevice, which is the actual handle.

2. I can definitely see the point of not providing two different way for the same task. I will chose to make use of a dedicated function, because it gives you more flexiblity like changing the device while recording. If start() is called alone, the default device will be used. If setDevice() will be called before or after, the device specified will be used. I don't understand what needs to be clarified in getCurrentDeviceName()? Could you explain that again?

3. Indeed you're absolutly right. I'll add a call to onStop() in setDevice().

I'll try to implement the changes tomorrow.
Title: Re: Help with the design for selecting audio capture device
Post by: Laurent on September 23, 2013, 07:47:11 am
Quote
But I think for variable names deviceName should be used instead of device, so it won't be confused with captureDevice, which is the actual handle.
Ah, you're right :)

Quote
I don't understand what needs to be clarified in getCurrentDeviceName()? Could you explain that again?
What I meant is: does it return an empty string when not recording, or the name of the last device used?
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on September 23, 2013, 02:19:39 pm
I see what you mean now! Well I'd say if it's not recording it should return the name of the device that was last specified. If none was set before it will return the name of the default device.
Title: Re: Help with the design for selecting audio capture device
Post by: Laurent on September 23, 2013, 02:57:27 pm
Sounds good.
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on September 23, 2013, 04:40:49 pm
Alright! I implemented the changes/bug fixes.
Can anybody think of something else I might be missing?
Title: Re: Help with the design for selecting audio capture device
Post by: Laurent on September 23, 2013, 04:48:10 pm
getAvailableDevices() should be static.
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on September 23, 2013, 04:56:35 pm
Indeed indeed...
It is now :)
Title: Re: Help with the design for selecting audio capture device
Post by: Laurent on September 23, 2013, 07:05:01 pm
I think it looks very good now, let me do a final review and it should be ok for integration ;)

In the meantime, others can still give some feedback, it is always helpful.
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on September 23, 2013, 08:58:12 pm
Sounds awesome!

I would love to hear some feedback from other people too!
Title: Re: Help with the design for selecting audio capture device
Post by: Hiura on September 23, 2013, 10:14:44 pm
Very nice work! :-)

sf::VideoMode has getFullscreenModes and getDesktopMode. It might also be a good idea to have sf::SoundRecorder::getDefaultDevice in addition to getAvailableDevices, don't you think?

But what happens when there is no recording device? Returning "" would be the only viable option, I guess. In any case isAvailable should be used first so it shouldn't matter much.

On a broader thought, both start and setDevice can fail. But the user has no idea when it fails or not, nor is there a isRecording method while sf::Music has getStatus. What do you think of returning a boolean from start and setDevice / adding sf::Sound[Buffer]Recorder::isRecording?

I never really used SFML's recording feature so let me know if those ideas make sense for you.  ;)
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on September 24, 2013, 02:00:12 am
Thank you very much for your input! Some of your ideas actually make a lot of sense!

I am not to sure about getDefaultDevice(), because you can simply select the default device by calling setDevice("") with an empty string as a parameter. But it might still be a good idea to implement such a function, for consistency (as you pointed out) and to give the user the ability to find out what device is the default device (there is no garanty that the first element in the list is also the default one). We'll see what the others say.

Quote
But what happens when there is no recording device?
I don't really understand what you mean by that. Do you mean the return of getCurrentDeviceName()? It always returns the name of the currently set device or is none is set the default device name. (Actually it didn't in all cases, but I fixed that just now :D)

About the missing feedback on the success of starting a capture. You are absolutly right! It seems weird that all the loadFrom.../create methods from the other classes return a bool that show if something failed. start() and setDevice() should definitly do that too. Right now there is just an error message, but the user has no idea if it's recording or not.
I would also like to implement your suggestion of a isRecording() method for sf::SoundRecorder because it allows you to have it as a loop-condition and to get what state the recorder is currently in. Also it's consistent with sf::Music.
But I'd like to hear what the others say about those things.

Thanks again!
Title: Re: Help with the design for selecting audio capture device
Post by: Laurent on September 24, 2013, 08:01:39 am
There is always a valid device, so there's no "no device" state to return. Unless the target system has no recording feature, in which case SoundRecorder::isAvailable() will return false and getDevice() can return an empty string.

The isRecording() function is useless (especially as a loop condition): the recording doesn't stop by itself, it is active only between the calls to start() and stop(). So the user always knows when the recording is active, since he controls it.

Returning a status in start() and setDevice() is needed, yes :)

There's something else that feels wrong now with all these additional functions: since this class is meant to be derived, all the statis functions will be available from all the derived classes. And we'll get SoundBufferRecorder::getAvailableDevices(), MyRecorder::getDefaultDevice(), etc. Although it's perfectly ok, it feels weird. In SFML 3 I may split the recording from the notifications, something like:

SoundRecorder::setDevice(SoundRecorder::getDefaultDevice());

MyRecorderObserver observer; // need a better name :)
SoundRecorder::start(observer);
SoundRecorder::stop();
Title: Re: Help with the design for selecting audio capture device
Post by: Hiura on September 24, 2013, 11:29:08 am
I am not to sure about getDefaultDevice(), because you can simply select the default device by calling setDevice("")

I missed that part of the documentation (I mean, using "" to select the default device). However, it's a little bit tricky to get the default device with the current proposed interface. Let's say I want to display a list of all devices to the user so he can select the one he wants, and I want to highlight the default one. I could do it like this:

auto defaultName = getDefaultDevice();
auto allDeviceNames = getAvailableDevices();
*find(allDeviceNames.begin(), allDeviceNames.end(), defaultName) = defaultName + " - default";
presentNamesToTheUser(allDeviceNames);
 

and implement getDefaultDevice() as follow...

{
    auto dummyRecorder = SoundRecorder();
    // dummyRecorder.setDevice(""); // Just to be sure -- Wait! It won't work with the current impl.
    return dummyRecorder.getCurrentDeviceName();
}
... but it feels wrong.  ;)

Quote
But what happens when there is no recording device?
I don't really understand what you mean by that. Do you mean the return of getCurrentDeviceName()?
In fact I was talking about sf::SoundRecorder::getDefaultDevice(). So it's not a big deal since there is sf::SoundRecorder::isAvailable().


The isRecording() function is useless (especially as a loop condition): the recording doesn't stop by itself, it is active only between the calls to start() and stop(). So the user always knows when the recording is active, since he controls it.
I guess it's not the most useful feature since there was no feature request for it, indeed.
Title: Re: Help with the design for selecting audio capture device
Post by: Laurent on September 24, 2013, 11:53:40 am
If we have a getDefaultDevice() function then we should remove the "" shortcut for it in the public API, it is not relevant anymore.

recorder.setDevice(SoundRecorder::getDefaultDevice());
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on September 24, 2013, 06:12:40 pm
Alright those are a lot of new things! I'll try to sort them and answer them all.

1. getDefaultDevice()
Quote
implementation of getDefaultDevice() [...] feels wrong
Indeed if it would be implemented like that (which wouldn't work because sf::SoundRecorder has to be derived), it would feel more wrong! Luckly OpenAL has a method that returns the name of the default capture device. I'll try to implement getDefaultDevice() later.
Quote
[..]we should remove the "" shortcut[...]
Sadly this is not possible, because if alcCaptureOpenDevice() is called with an empty string it will use the default device. Theoretically we could check if setDevice() is called with an empty string and output an error message (recommending the use of getDefaultDevice()). But I'm not sure if it's worth the effort. Maybe simply not documenting it should be enough.


2. Return a status in start() and setDevice()
I'll try implement that later today.


3. isRecording()
Quote
The isRecording() function is useless[...]
Yeah you're right. Since there is no outside "event" (like file ended in sf::Music) the state of the recording is always obvious.


4.
Quote
feels wrong [...] all the status functions will be available from all the derived classes[...]
Why not make them standalone functions then?


Alright I hope I didn't miss anything. If I did please remind me! I'll try to implement the getDefaultDevice() and the return of the status later today.
Thanks again for the valueable feedback and the help!
Title: Re: Help with the design for selecting audio capture device
Post by: Hiura on September 24, 2013, 07:11:25 pm
I think you summed up everything.  :-)

Quote
Quote
[..]we should remove the "" shortcut[...]
Sadly this is not possible
I think Laurent was referring more to the documentation than the implementation. For me it would be a valid design to state in the doc that the device name should be in the list returned by getAvailableDevices() and, without specifying it in the doc, that the implementation also accepts the empty string without complaining – after all it's up to the user to make sure the identifier is valid.
Title: Re: Help with the design for selecting audio capture device
Post by: Laurent on September 24, 2013, 08:54:46 pm
Quote
Maybe simply not documenting it should be enough.
That's what I meant :)

Quote
Why not make them standalone functions then?
Same result, a little worse: instead of "duplicating" the functionality in each derived class, you have it duplicated in every instance of each derived class. As I said, a better design would have it in only one place. But that's a different story (for SFML 3).

EDIT: sorry I misread your answer, forget my last comment. I read "non-static" where you wrote "standalone" ;D
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on September 25, 2013, 04:43:40 pm
Ok I just implemented the latest suggestions. If nobody else has anything to add I think we are ready for the final review :D

Quote
sorry I misread your answer
So what are you thinking about making them standalone? (Even if we leave that for SFML 3, to not break the API)
Title: Re: Help with the design for selecting audio capture device
Post by: Hiura on September 25, 2013, 05:21:05 pm
Looks nice.  :)

Except for one tiny thing: getDefaultDevice() should be static.
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on September 25, 2013, 06:26:08 pm
Thank you!

I changed it.
Title: Re: Help with the design for selecting audio capture device
Post by: Laurent on September 25, 2013, 08:56:16 pm
Quote
So what are you thinking about making them standalone?
Standalone like free functions? No, but they could definitely be separated from the notification functions (i.e. in another class), like in what I've shown earlier.

I'll try to review your modifications as soon as possible. I already have a comment: getCurrentDeviceName() should be named getDevice().
Title: Re: Help with the design for selecting audio capture device
Post by: Nexus on September 25, 2013, 09:14:02 pm
Some minor issues: You sometimes use an explicit std::string conversion although unnecessary (const char* can be implicitly converted). Some corrected examples, there are more:
m_deviceName = alcGetString(captureDevice, ALC_CAPTURE_DEVICE_SPECIFIER);
...
deviceNameList.push_back(deviceList);
...
return alcGetString(NULL, ALC_CAPTURE_DEFAULT_DEVICE_SPECIFIER);

You should also pass std::string parameters by const reference instead of value.

By the way, shouldn't m_isCapturing be protected by a mutex? Even though it's just a bool, read/write is not guaranteed to be atomic.
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on September 25, 2013, 09:22:46 pm
Oh I didn't know there was an implicit conversion for std::string. I'll change that. And also so the const reference instead to value.

Since SFML is using C++03 there are no atomic operations, but I think I once heard that storing/getting of a bool can usually be assumed to be safe.

I already changed the name to getDevice(). I'll see if I find some time to do the other suff later on.
Title: AW: Re: Help with the design for selecting audio capture device
Post by: eXpl0it3r on September 25, 2013, 09:29:06 pm
Since SFML is using C++03 there are no atomic operations, but I think I once heard that storing/getting of a bool can usually be assumed to be safe.
"usually safe" implies that it's not safe in all cases, thus you're running the risk of possible race conditions etc. ;)
It may work in 98% of all cases, but it will be that 2% who will open up a tickt in the future. :P
Title: Re: Help with the design for selecting audio capture device
Post by: Nexus on September 25, 2013, 09:29:31 pm
but I think I once heard that storing/getting of a bool can usually be assumed to be safe.
Not really, otherwise the std::atomic_bool (http://en.cppreference.com/w/cpp/atomic/atomic) type would be pretty pointless.
Title: Re: Help with the design for selecting audio capture device
Post by: Hiura on September 25, 2013, 11:21:08 pm
Regarding the code I spot two minor things: getDefaultDevice() should be directly after getAvailableDevices(), and getDevice() should be directly before/after setDevice(). Then the initialisation of m_deviceName could use getDefaultDevice() instead of duplicating one line of code.

Regarding potential data races in this class, if we assume the user will use a recorder's API on one thread only (or on multiple thread but with a proper use of mutex), having in mind that all attributes are private, we can say the following:


However, there is an undocumented (I think) possible data race on captureDevice / alcCaptureOpenDevice() when start() is called on two different recorders from two different threads. I haven't looked at the documentation of alcCaptureOpenDevice() so I can only speculate that one of the two recorders will fail if the implementation of OpenAL is thread safe. Otherwise we could simply add in the documentation of start() that this situation results in an undefined behaviour.

(*) it would be good I think to add in the documentation that getSampleRate() is valid / meaningful only after start() is called since it's not set by the user in the constructor of the class.

Let me know if you think my interpretation of the code is right, or wrong.
Title: Re: Help with the design for selecting audio capture device
Post by: Laurent on September 26, 2013, 07:51:22 am
Quote
However, there is an undocumented (I think) possible data race on captureDevice / alcCaptureOpenDevice() when start() is called on two different recorders from two different threads.
The code explicitly disallows this situation, only one active recording sessions is allowed, the second call to start() would fail (but there's probably a need for a mutex there too).
Title: Re: Help with the design for selecting audio capture device
Post by: Hiura on September 26, 2013, 09:35:05 am
Yes, that's what I'm talking about. The test is in an (unsecure) critical section. There is two solutions; either using a mutex, or adding a comment in the documentation.
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on September 26, 2013, 08:57:12 pm
I implemented the proposed changes.
I am not an expert in multithreading, so if I should implement a mutex around captureDevice someone needs to show me how.
Other than that does anybody have another comment about the code?
Title: Re: Help with the design for selecting audio capture device
Post by: Laurent on September 26, 2013, 10:09:58 pm
Concurrent access issues are another problem. Don't try to solve everything in your pull request, focus on the initial issue.
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on September 26, 2013, 10:16:35 pm
Alright! Then if nobody else has anything to add I think the initial issue is fixed :)
Title: Re: Help with the design for selecting audio capture device
Post by: Laurent on September 26, 2013, 10:25:13 pm
Thanks. I'll do a final review as soon as possible :)
Title: Re: Help with the design for selecting audio capture device
Post by: Laurent on September 27, 2013, 03:06:51 pm
It looks good, so now let's fix the cosmetic issues ;)

There are many tiny issues in your comments. It would take ages for me to describe them in detail, so I let you do a first pass yourself. Mainly: missing commas, periods, "while while",  "if(", ...

And I would modify the description of setDevice: the function doesn't open the capture device, it just changes the active device for following captures.

This is really tedious but it would be really ugly to merge a modification and then add a commit just to fix minor stuff like this. Hmm... pull requests are great but they start to consume as much of my time as if I did the modification myself :P

By the way, what do the device names returned by OpenAL look like?
Title: Re: Help with the design for selecting audio capture device
Post by: Nexus on September 27, 2013, 03:56:13 pm
This is really tedious but it would be really ugly to merge a modification and then add a commit just to fix minor stuff like this. Hmm... pull requests are great but they start to consume as much of my time as if I did the modification myself :P
Interesting, back then (http://en.sfml-dev.org/forums/index.php?topic=10589.msg73434#msg73434) no one believed me :D

Maybe you could check out Foaly's branch from your hard drive, add more commits locally, merge and push them. The web interface is just too limited for anything beyond the most basic usage. When you click on its green merge button, isn't every single commit of Foaly added to your history? If so, that might not be what you want anyway. Manually, you could squash the commits and clean everything up. If it's not possible to keep Foaly as an author, you can credit him in the commit message...
Title: Re: Help with the design for selecting audio capture device
Post by: Laurent on September 30, 2013, 11:01:37 pm
Yes, I remember this discussion ;)

I still prefer doing it this way, even if it becomes time-consuming and a little frustrating. At least the request be merged eventually, and next time Foaly contributes (I'm sure he will :))), it will be perfect.

Nice new logo, by the way.
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on October 01, 2013, 11:30:55 am
Sorry about the holdup, I was busy studying for an exam.
Anyway I reread the documentation and corrected all the mistakes I could find. If you can see any other ones please let me know and I'll fix them.

Quote
By the way, what do the device names returned by OpenAL look like?
I have a USB Interface (soundboard) that looks like that: Mikrofon (2- USB Audio CODEC) and a webcam that looks like that: Mikrofon (HD Webcam C310) (those are the strings returned by getAvailableDevices().
Title: Re: Help with the design for selecting audio capture device
Post by: Laurent on October 01, 2013, 11:52:08 am
Thanks :)

I noticed that, in case of a call to setDevice(""), getDevice() will return "" until the next capture starts, and the actual default device name after (even after the capture was stopped). If we don't call setDevice(""), getDevice() will always return the actual name of the default device. This feels a little inconsistent. Maybe you should retrieve the name of the default device in setDevice if the argument if an empty string, and then remove the calls to alcGetString(captureDevice, ALC_CAPTURE_DEVICE_SPECIFIER) in start and setDevice.

Also, what happens if the default device changes (I guess it can be changed in the OS control panel -- or, if you can set your USB webcam as the default device, it can be hot-plugged/unplugged) during runtime? Does the empty string correctly map to the new default device, or does OpenAL always returns the same initial default device? Would you be able to test this use case?
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on October 01, 2013, 03:52:34 pm
That is indeed a bug! :D I fixed it. The name of the default device is now stored if setDevice() is called with an empty string.

I did some test with hot-plugging the devices and it seems like OpenAL doesn't map getDefaultDevice() correctly to the next device, if you unplug the default device. It uses the initial default device. Of course if you try to keep on using it there are OpenAL errors. The list of getAvailableDevices() on the other hand is always up to date.
Title: Re: Help with the design for selecting audio capture device
Post by: Laurent on October 01, 2013, 04:13:01 pm
Bug: if calling setDevice while recording fails, m_deviceName will never be updated.
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on October 01, 2013, 10:45:43 pm
I am not sure if I'd call that a bug. I think it is the expected behaviour. If the setDevice() fails, the selected device fails to open and captureDevice is not set, so why store the name?
Title: Re: Help with the design for selecting audio capture device
Post by: Laurent on October 01, 2013, 10:56:52 pm
Because if you were not in the middle of a capture, the device is assigned and all subsequent captures will fail until it is changed again.
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on October 01, 2013, 11:01:49 pm
Ok I updated the code.
Title: Re: Help with the design for selecting audio capture device
Post by: Laurent on October 02, 2013, 10:20:12 pm
Why don't you just move the "// Store the device name" block at the beginning of the function? The current code is inconsistent, you don't store the same thing in both cases (if an empty string is passed).
Title: Re: Help with the design for selecting audio capture device
Post by: Nexus on October 03, 2013, 01:38:25 pm
Sorry for offtopic, but I just stumbled about new GitHub features which intend to make pull requests easier to process (if you use the Git for Windows application). It's not ideal, but might still be worth reading.
Nice new logo, by the way.
Thanks! I found it's time to give my whole website a new look :)
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on October 04, 2013, 01:00:19 pm
Sorry I won't be near my computer for a couple days. I'll try to fix it next week!
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on October 07, 2013, 11:47:07 pm
OK it is consistend now! :) sorry about that I was distracted when I wrote the last fix...
Title: Re: Help with the design for selecting audio capture device
Post by: Laurent on October 08, 2013, 07:01:15 pm
Make getDevice return a const reference, merge everything into a single clean commit, and I'll integrate it. Finally :P
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on October 08, 2013, 09:01:06 pm
Awesome! :D
I did the first thing, but I don't know how to merge the whole pull request into a single commit. I only know some basic git commands, so if somebody could explain to me how it's done, that would be great!
Title: Re: Help with the design for selecting audio capture device
Post by: Nexus on October 08, 2013, 09:13:08 pm
git rebase -i <rev>, where <rev> is the first of your commits. When the text editor opens the file, replace all "pick" words by "squash", store the file and close it.

You may need to force-push (git push -f) to rewrite the history to GitHub.
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on October 08, 2013, 09:35:32 pm
Ok. I'm not sure I understood everything, but I'll try. How to I get the <rev>?

edit: Would it be this one: 005396d32f3f302560e2553dc26acb60f4952b33 (https://github.com/Foaly/SFML/commit/005396d32f3f302560e2553dc26acb60f4952b33)
Title: Re: Help with the design for selecting audio capture device
Post by: Nexus on October 08, 2013, 10:27:40 pm
<rev> is the hash of the first commit you made.

It suffices to enter the first 5-7 characters of the hash, as long as it is not ambiguous.
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on October 08, 2013, 11:08:51 pm
I used git rebase -i 005396d32 and replaced every "pick" with "squash". I got the error message: Can not 'squash' without previous commit.
Do I have to leave the first pick and only make every following to a squash?
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on October 09, 2013, 12:09:40 am
I don't know... I tryed for the last hours and I think I got it right now, excpet that I have to find out how to change the commit message  :D
Title: Re: Help with the design for selecting audio capture device
Post by: Nexus on October 09, 2013, 12:43:34 am
Yes sorry, there must be at least one "pick" (the first one). Thus, <rev> is the last commit before yours, so the rebase text file will contain all of your commits.

Use git commit --amend to change the last commit, or git rebase -i with "reword" instead of "pick".
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on October 09, 2013, 08:48:24 am
Alright I changed the commit message.
I'm not 100% sure that I did everything right, because there where some merge conflicts that I had to resolve. I hope I did everything right, but if somebody could have a look at it again, that would be great!
Other than that I think we are good to go now :D and thanks for the help!
Title: Re: Help with the design for selecting audio capture device
Post by: Hiura on October 09, 2013, 08:51:33 am
Nice. There are still two commits but it's already better. I think you can rebase with a branch too (instead of a commit sha). Something like `git rebase -i master` and squash the second commit into the first one.
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on October 09, 2013, 09:22:50 am
I tryed that and now everything is in one commit. I think we are good to go now!  ;D
Thanks again!
I found a site  (http://blog.steveklabnik.com/posts/2012-11-08-how-to-squash-commits-in-a-github-pull-request)that shows how to squash all commit of a pull request into one (of course you always find those things after you're done... but maybe it's helpful for the next person).
Title: Re: Help with the design for selecting audio capture device
Post by: Laurent on October 10, 2013, 07:49:41 am
I think something is wrong. Look at setDevice for example, it's not the latest version of the code.

I hope you kept a local copy of the code before you merged everything :P
Title: Re: Help with the design for selecting audio capture device
Post by: Nexus on October 10, 2013, 08:12:39 am
I hope you kept a local copy of the code before you merged everything :P
Otherwise, there's still git reflog ;)
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on October 10, 2013, 10:29:51 am
For fuck sacks... I knew something like that would happen! Of course I didn't back the files up before I merged them. How can I restore them?

edit: I took a look at setDevice again and I don't noticed anything wrong. What did you see that is wrong?
Title: Re: Help with the design for selecting audio capture device
Post by: Laurent on October 10, 2013, 06:38:02 pm
The latest code was:
if (device.empty())
    m_deviceName = getDefaultDevice();
else
    m_deviceName = device;

...
Title: Re: Help with the design for selecting audio capture device
Post by: Nexus on October 10, 2013, 07:17:41 pm
How can I restore them?
Read my post ;)
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on October 10, 2013, 08:17:47 pm
Laurent are you 100% sure it was like that? Because I am almost certainly sure, that there was never a variable device (because of the confusion with the actual device handle). I always used name or deviceName. And also the latest commit moved the // Store name block to the top of the method, where it is now. So maybe there is nothing wrong.
Title: Re: Help with the design for selecting audio capture device
Post by: Nexus on October 10, 2013, 09:00:34 pm
Of course you can also guess, if you prefer that to checking the reflog.
Title: Re: Help with the design for selecting audio capture device
Post by: Laurent on October 10, 2013, 09:18:43 pm
Indeed it looks ok. I must have looked at the wrong commit. But you'd better be 100% sure, manual checking won't be enough, there were so many changes to the code.
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on October 11, 2013, 12:38:08 am
So what should I do?
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on October 11, 2013, 11:20:28 am
I just checked the whole commit (https://github.com/Foaly/SFML/commit/2510b0edb61435c5b83456c0f273350852b7382f) again and I am sure that everything is in it.
Title: Re: Help with the design for selecting audio capture device
Post by: Foaly on October 11, 2013, 06:56:44 pm
Awesome! Thank you very much Laurent for all the patience and help! Also thanks to nexus for the git help an to everybody else who helped! Now I can switch from portaudio back to SFML for my sound processing program. Also it was a lot of fun to contribute to SFML!
Title: Re: Help with the design for selecting audio capture device
Post by: zsbzsb on October 11, 2013, 09:00:32 pm
Laurent, any chance of getting this added to CSFML (and then SFML.NET)?
Title: Re: Help with the design for selecting audio capture device
Post by: eXpl0it3r on October 11, 2013, 09:07:44 pm
Laurent, any chance of getting this added to CSFML (and then SFML.NET)?
Well you know where those repositories are, so... :P
Title: Re: Help with the design for selecting audio capture device
Post by: zsbzsb on October 11, 2013, 09:16:40 pm
Well you know where those repositories are, so... :P

I will look into it later if Laurent is not already working on it  ;)
Title: Re: Help with the design for selecting audio capture device
Post by: Laurent on October 11, 2013, 10:26:01 pm
I haven't updated CSFML and SFML.Net since the 2.1 release. I was planning to make a big update later.

Feel free to send a pull request if you need it sooner.