Skip to content

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

Merged
alalek merged 7 commits intoopencv:4.xfrom
MaximMilashchenko:AudioUpdate
Dec 4, 2021
Merged

Audio MSMF: added the ability to set sample per second#21145
alalek merged 7 commits intoopencv:4.xfrom
MaximMilashchenko:AudioUpdate

Conversation

@MaximMilashchenko
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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