Opened 7 years ago

Closed 5 years ago

#8911 closed enhancement (fixed)

Write a new BSoundConsumer class to replace the SoundConsumer class

Reported by: korli Owned by: hamish
Priority: normal Milestone: R1
Component: Kits/Media Kit Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

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

Attachments (5)

soundrecord.diff (32.4 KB) - added by Barrett 6 years ago.
Patch to replace SoundConsumer with BMediaRecorder.
0001-Replaced-SoundConsumer-with-MediaRecorder.patch (59.5 KB) - added by Barrett 6 years ago.
Patch to replace SoundConsumer with BMediaRecorder.
0001-Replaced-SoundConsumer-with-BMediaRecorder.patch (59.6 KB) - added by Barrett 5 years ago.
0001-Replace-SoundCounsumer-with-modified-BMediaRecorder.patch (59.9 KB) - added by Barrett 5 years ago.
0001-BMediaRecorder-fix-a-few-issues-fix-style-remove-unu.patch (25.6 KB) - added by hamish 5 years ago.

Download all attachments as: .zip

Change History (28)

comment:1 Changed 7 years ago by 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
Last edited 7 years ago by hamish (previous) (diff)

comment:2 Changed 7 years ago by 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.

Version 0, edited 7 years ago by leavengood (next)

comment:3 Changed 7 years ago by anevilyak

Owner: changed from axeld to hamish
Status: newassigned

Done.

comment:4 Changed 7 years ago by diver

Version: R1/alpha3R1/Development

comment:5 Changed 6 years ago by Barrett

Has a Patch: set

comment:6 Changed 6 years ago by 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....

comment:7 Changed 6 years ago by 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"?

comment:8 Changed 6 years ago by waddlesplash

Why is this a private API anyways?

comment:9 Changed 6 years ago by Barrett

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

Changed 6 years ago by Barrett

Attachment: soundrecord.diff added

Patch to replace SoundConsumer with BMediaRecorder.

comment:10 Changed 6 years ago by 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.

comment:11 Changed 6 years ago by 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?

comment:12 Changed 6 years ago by 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!

Changed 6 years ago by Barrett

Patch to replace SoundConsumer with BMediaRecorder.

comment:13 Changed 6 years ago by 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.

comment:14 Changed 6 years ago by 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 :)

comment:15 Changed 6 years ago by 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.

comment:16 Changed 5 years ago by 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.

comment:17 Changed 5 years ago by Barrett

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

comment:18 Changed 5 years ago by Barrett

Could someone please look at it? Thanks.

comment:19 Changed 5 years ago by 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.

comment:20 Changed 5 years ago by Barrett

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

comment:21 Changed 5 years ago by 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?

comment:22 Changed 5 years ago by hamish

Patch attached with fixes and removal of SoundUtils

comment:23 Changed 5 years ago by hamish

Resolution: fixed
Status: assignedclosed

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

Note: See TracTickets for help on using tickets.