Skip to content

Audio MSMF: added the ability to set sample per second #21145

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Dec 4, 2021

Conversation

MaximMilashchenko
Copy link
Contributor

@MaximMilashchenko MaximMilashchenko commented Nov 28, 2021

I changed the CAP_PROP_AUDIO_SAMPLES_PER_SECOND property from (read-only) to (open, read). Acceptable values are 8.0 kHz, 11.025 kHz, 22.05 kHz, and 44.1 kHz. If the value is not set, the behavior is standard.
@alalek

force_builds=Win32

Comment on lines 1326 to 1330
if (value != 8000 && value != 11025 && value != 22050 && value != 44100)
{
CV_LOG_ERROR(NULL, "VIDEOIO/MSMF: CAP_PROP_AUDIO_SAMPLES_PER_SECOND parameter value is invalid/unsupported: " << value);
return false;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe MSMF framework should know better which data rate is supported. There is no limitation on OpenCV side (except may be >= 0 validation check). No need to duplicate MSMF logic in OpenCV code - no way to maintain this approach.
Need to ensure that MSMF should properly return "false" from .open() with unsupported parameter value.

Copy link
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add test with "invalid" / "non-supported" values.
cap.isOpened() should return false if .open() parameters are not supported and/or not compatible.

if (isOpen)
{
if (audioStream != -1)
checkAudioProperties();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checkAudioProperties();

We should not ignore return value.
Function returns false on errors. Perhaps we should .close capture object in that case.

When is configureAudioOutput() called in this flow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In open, first called configureOutput, then inside this function called configureAudioOutput. If I understood the question correctly

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad. This note is about setAudioProperties() call above (line 1154)

{
CV_LOG_ERROR(NULL, "VIDEOIO/MSMF: CAP_PROP_AUDIO_SAMPLES_PER_SECOND parameter value is invalid/unsupported: " << audioSamplesPerSecond
<< ". Current value of CAP_PROP_AUDIO_SAMPLES_PER_SECOND: " << actualAudioSamplesPerSecond);
captureAudioFormat.nSamplesPerSec = actualAudioSamplesPerSecond;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

captureAudioFormat.nSamplesPerSec = actualAudioSamplesPerSecond;

We should call close() here because we can't satisfy requested parameters.

Copy link
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you 👍

@alalek alalek merged commit cb08c15 into opencv:4.x Dec 4, 2021
@alalek alalek mentioned this pull request Dec 30, 2021
@MaximMilashchenko MaximMilashchenko deleted the AudioUpdate branch January 11, 2022 14:27
@alalek alalek mentioned this pull request Feb 22, 2022
a-sajjad72 pushed a commit to a-sajjad72/opencv that referenced this pull request Mar 30, 2023
Audio MSMF: added the ability to set sample per second

* Audio MSMF: added the ability to set sample per second

* changed the valid sampling rate check

* fixed docs

* add test

* fixed warning

* fixed error

* fixed error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants