Write a new BSoundConsumer class to replace the SoundConsumer class

[korli]

The class SoundConsumer from src/kits/media/SoundConsumer.cpp and headers/private/media/SoundConsumer.h is to be rewritten

• to avoid license issues
• clean/fix the API
• provide virtual slots.

It should also be more in line with other media kit classes, ie use the debug macros defined in headers/private/media/debug.h

[hamish]

If no one else is working on this, I'd like to.

Regarding the cleaner API, I think BSoundConsumer should be made more analogous to BSoundPlayer, i.e.:

• registering and connecting the nodes and setting the time source should be handled by the constructor, which should take a media_output parameter
• starting the node and time source (if necessary) should be done in BSoundConsumer::Start

[leavengood]

Having a similar API for two "mirror" classes like BSoundPlayer and BSoundConsumer definitely makes sense.

If Niels is listening maybe he can add you as a developer so you can take ownership of this issue and any others you want to work on.

I think we could probably also you get commit access fairly quickly to the Haiku repo.

[anevilyak]

Done.

[Barrett]

I talked some time ago with hamish about doing this ticket, so in the latest days i've finally got the time to do it. I've actually take a class from the beos sample code, it's the MediaRecorder. The patch include a modified version of it, with some improvements and cleanup. I've also rewritten the header file, setting it's copyright to Haiku Inc.

The main advantage over the BSoundConsumer class is that it support any type of media, it's implementation is very simple and effective, this will allow us in future to improve it easily. I've also removed some code from SoundRecorder since it wasn't needed anymore. The idea is to do a set of patches to improve SoundRecorder, before all i would like to separate a bit more the audio from gui and then rewrite the code to share it's controls with MediaPlayer. I'm also planning to fix style in the process. I've actually tried to do some of it in GCI but i've had no students taking this task.

So i would like to release the work as a set of patches, so this is the first part. I'm currently in test phase. but the app looks like it's working perfectly, so i would like to know if there is any comment or suggestion....

[Barrett]

I've updated the patch to set some methods as const and fix some style in the includes. Actually i'm not having care about virtual slots, the only virtual method is BufferReceived which is meant to be inherited. I suppose i have to do it also for Connect, Disconnect and Start/Stop?

Is it safe to replace the "foo < B_OK" with "foo != B_OK"?

[waddlesplash]

Why is this a private API anyways?

[Barrett]

My idea was that it should be refined/tested a bit before to become a public API. Before R1 at least...

[Barrett]

Patch to replace SoundConsumer with BMediaRecorder.

[Barrett]

I've fixed the issues of my previous post. After some more testing i can't find any regression compared with the SoundConsumer version. BMediaRecorder is doing his work as well. Hope someone will look at it.

[hamish]

Hi Barrett,

Thanks for the patch! Haven't looked very deeply yet but there's some issues I can see:

• Input nodes retrieved from the roster should be released upon disconnection.
• Local variable and function naming style is sometimes wrong (shouldLookLikeThis).
• NULL should be used for null pointers, not 0.
• snprintf should be used instead of sprintf.
• Opening brace of class definition should be on the same line i.e. class BMediaRecorder {

Also, why provide a function for accessing the BMediaRecorder's internal node?

[Barrett]

Thanks for looking at it, i've fixed the issues you listed and made some more massaging to the code. About the method returning the media_node, i've think that the class at some point will support also "external" connections made without using the BMediaRecorder::Connect(). But it can be done using the MediaOutput() method, so i removed it as you suggested.

I've also added some debugging calls to make it more in line with other mediakit classes, if you have other suggestions just let me know!

[Barrett]

Patch to replace SoundConsumer with BMediaRecorder.

[Barrett]

I've just realized that I've inadvertently changed the style of two pointers in the recorder window, so i sent an updated patch.

[korli]

Hi Barrett! Thanks for the patch! I know the code before might not be coding style compliant and your patch already fixed some but not introducing new issues would be nice :)

• ProcessFunc and NotifyFunc should be placed in the class.
• no blank line after the copyright header in header files.
• two blank lines after the header guard.
• MediaRecorderNode.h, MediaRecorderNode.cpp and MediaRecorder.cpp don't use the usual copyright header.
• BMediaRecorder constructor parameter names don't match with BMediaRecorderNode.
• What is StNodeRelease? One should probably use/extend AutoDeleter.h. Parameter and members could use better names anyway.
• the protected scope constructors for BMediaRecorder have no implementations. Ditto for the equal operator.
• I don't understand why one should now provide a thread priority, as it should depend of the run mode and the media type. Use suggest_thread_priority() and implement SetRunMode(). One should implement media_realtime_init_thread() in the media kit someday :)

[Barrett]

Unfortunately as the latest month i've lost the wave, so i'll able to return on it in a couple of days.

I like the idea of a MediaNodeReleaser class, i'll rewrite StNodeRelease conforming to the AutoDeleter style.

About the unimplemented things, i think the equals operator doesn't have any usefulness, so i plan to just remove it.

[Barrett]

Hello, i've updated the patch with the changes you suggested. I've removed the unimplemented constructor because the only way i've think of it is to automatically connect to the system mixer, but seeing it not a lot logic i've removed it. The same for the copy constructor and equal operators, can't think of a reason to copy a BMediaRecorder.

I've used suggest_thread_priority only for B_OFFLINE because there wasn't an accurate variable for it since we can't know if the node will be used for audio or video recording for example. I'm not sure about what you meant for the default copyright header, i've just looked at other files using the be sample code license.

[Barrett]

I've refined a bit the patch, and the node now change it's priority depending on the run mode.

[Barrett]

Could someone please look at it? Thanks.

[hamish]

Hi Barrett,

I applied this patch locally a week or two ago and have been fixing a couple of issues - just haven't had a lot of time. I haven't forgotten about it though.

[Barrett]

Ok, i've just some code back relating to the ticket #10398, that's why i replaced soundconsumer with BMediaRecorder.

[Barrett]

At least give me a hint on those "couple of fixes" and i'll fix them. This is really a demotivating way of work for me. Eight months are a long time, isn't it?

[hamish]

Patch attached with fixes and removal of SoundUtils

[hamish]

Applied in hrev48668. The header is still private at the moment.