PR1 - Audio - Non silent region detection HOST Tensor #149

snehaa8 · 2023-08-03T15:11:54Z

Includes unittest setup

r-abishek · 2023-08-31T23:40:26Z

utilities/test_suite/HOST/audio/Tensor_host_audio.cpp

+
+    // Set case names
+    char funcName[1000];
+    switch (testCase)


@sampath1117 Things like this have to be rearranged. Cannot have switch case twice. Mods like those for image test suite needed here

r-abishek · 2023-08-31T23:41:48Z

utilities/test_suite/HOST/audio/Tensor_host_audio.cpp

+
+        // The SF_INFO struct must be initialized before using it
+        memset (&sfinfo, 0, sizeof (sfinfo));
+        if (!(infile = sf_open (temp, SFM_READ, &sfinfo)))


Same with the strategies we employed to avoid image reads twice

r-abishek · 2023-08-31T23:43:33Z

utilities/test_suite/HOST/audio/Tensor_host_audio.cpp

+            else
+                missingFuncFlag = 1;
+
+            verify_non_silent_region_detection(detectedIndex, detectionLength, testCaseName, noOfAudioFiles, audioNames);


We cannot add this to the Wall or CPU time @HazarathKumarM @sampath1117. End timer here, and then verify.

Where are we on this?

r-abishek · 2023-08-31T23:44:36Z

utilities/test_suite/HOST/audio/Tensor_host_audio.cpp

+
+void verify_non_silent_region_detection(float *detectedIndex, float *detectionLength, string testCase, int bs, char audioNames[][1000])
+{
+    fstream ref_file;


r-abishek · 2023-08-31T23:44:44Z

utilities/test_suite/HOST/audio/Tensor_host_audio.cpp

+void verify_non_silent_region_detection(float *detectedIndex, float *detectionLength, string testCase, int bs, char audioNames[][1000])
+{
+    fstream ref_file;
+    string ref_path = get_current_dir_name();


r-abishek · 2023-08-31T23:44:53Z

utilities/test_suite/HOST/audio/Tensor_host_audio.cpp

+    string pattern = "HOST/audio/build";
+    remove_substring(ref_path, pattern);
+    ref_path = ref_path + "REFERENCE_OUTPUTS_AUDIO/";
+    int file_match = 0;


r-abishek · 2023-08-31T23:45:44Z

utilities/test_suite/HOST/audio/Tensor_host_audio.cpp

+        Rpp32s ref_index, ref_length;
+        Rpp32s out_index, out_length;
+        ref_file>>ref_index;
+        ref_file>>ref_length;


Spaces before/after operator, and camelCase for variables

r-abishek · 2023-08-31T23:46:20Z

utilities/test_suite/HOST/audio/Tensor_host_audio.cpp

+            file_match += 1;
+        ref_file.close();
+    }
+    std::cerr<<std::endl<<"Results for Test case: "<<testCase<<std::endl;


Lets stick to any one standard, to use std::cerr or std::cout for any print statements everywhere

And spaces before/after operators

sampath1117

@snehaa8
Please address review comments

sampath1117 · 2023-09-01T05:58:56Z

src/modules/cpu/kernel/non_silent_region_detection.hpp

+    Rpp32u maxSrcLength = *std::max_element(srcLengthTensor, (srcLengthTensor + srcDescPtr->n));
+    Rpp32f *mmsBuffer = static_cast<Rpp32f *>(calloc(maxSrcLength, sizeof(Rpp32f)));
+
+    Rpp32f cutOff = std::pow(10.0f, cutOffDB * 0.1f);


Please remove the additional space at L53
Add const specifier for cutOff like const Rpp32f cutoff

sampath1117 · 2023-09-01T06:00:51Z

src/modules/cpu/kernel/non_silent_region_detection.hpp

+{
+    Rpp32u numThreads = handle.GetNumThreads();
+    Rpp32u maxSrcLength = *std::max_element(srcLengthTensor, (srcLengthTensor + srcDescPtr->n));
+    Rpp32f *mmsBuffer = static_cast<Rpp32f *>(calloc(maxSrcLength, sizeof(Rpp32f)));


Are we allocating a single mmsBuffer of size maxSrcLength for entire batch of audio samples?
if yes, this assumption is not correct and won't work as expected

Please change this

Fixed this
allocating a buffer inside batching loop itself was better in terms of perf.

sampath1117 · 2023-09-01T06:01:37Z

src/modules/cpu/kernel/non_silent_region_detection.hpp

+                                                  rpp::Handle& handle)
+{
+    Rpp32u numThreads = handle.GetNumThreads();
+    Rpp32u maxSrcLength = *std::max_element(srcLengthTensor, (srcLengthTensor + srcDescPtr->n));


Not very sure if this max calculation is also valid or not
@snehaa8 Have you verified the working of this?

This is valid, but removed it as we allocate mmsBuffer internally.

snehaa8 · 2023-09-08T18:32:19Z

Pulled in latest audio testsuite changes as done by Hazarath

I am not sure if review comments for testsuite added in this PR have been addressed.
I am still seeing
Error: libsndfile must be installed to install test_suite/HOST successfully!
"sudo apt-get install libsndfile1-dev" as usual helped
Not sure if this is expected
Need to add readme for Audio QA testsuite.
On running
python3 runAudioTests.py --qa_mode 1 --batch_size 8
Attaching output file here
I see passed for NSR which is expected but then I see
QA mode can only run with a batch size of 1.
towards end of log file.
I have shared corresponding log file via slack. @HazarathKumarM

These were my observations so far.

r-abishek · 2023-09-11T19:47:25Z

@HazarathKumarM please address audio test suite related review, and comment on the ones been addressed.
@snehaa8 libsndfile dependency for test suite is in the ReadMe change in this PR - https://github.com/r-abishek/rpp/pull/149/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5
@sampath1117 Please review if audio test suite's ReadMe is clear to the user - for current unit/qa/perf test support offered

r-abishek · 2023-09-11T19:50:47Z

utilities/test_suite/HOST/Tensor_host.cpp

@@ -187,10 +187,10 @@ int main(int argc, char **argv)

    // Get number of images and image Names
    vector<string> imageNames, imageNamesSecond, imageNamesPath, imageNamesPathSecond;
-    search_jpg_files(src, imageNames, imageNamesPath);
+    search_files_recursive(src, imageNames, imageNamesPath, ".jpg");


Do we need to touch the non-audio tests in this PR? (Tensor_host.cpp and Tensor_hip.cpp changes)

We have made the recursive file search generic, so that valid extensions are passed as input, so that it can be used for both image and audio

@HazarathKumarM May be you can issue this minor change as seperate PR to abishek fork and this can be merged as part of test suite PR3 itself. Since that PR is currently open, we can may be add this change, so that this change won't show up in audio PR

We have made changes to use same recursive file search function for both audio and image, so we are passing extensions as input to the function

If this change has a specific reason why its in the Audio PR, that's okay.
But if there are no dependencies for Tensor_host.cpp and Tensor_hip.cpp, would suggest putting it in the Test suite upgrade 3 PR.

sampath1117

@HazarathKumarM
Please address review comments

Also downmix and 3 sample wav files are also added in this PR. Please remove them

sampath1117 · 2023-09-12T07:29:24Z

.gitignore

@@ -12,6 +12,8 @@ hip_build/
 OUTPUT_IMAGES*
 OUTPUT_PERFORMANCE*
 QA_RESULTS*
+OUTPUT_AUDIO*


Why output audio is added?
we are not generating any audio outputs right?

When we discussed we plan to log the PASS OR FAIL IN an text file right
I am adding that txt file in OUTPUT_AUDIO* folder

@sampath1117 can you please confirm this

@r-abishek For image unit tests we are having output folder as OUTPUT_IMAGE_HOST , so for the audio I am using OUTPUT_AUDIO_HOST
is it Okay or we need some different folder name?

Similar folder name is fine yes, but only if its necessary to store actual outputs in it.
I was thinking PASS/FAIL is part of QA testing logged inside QA_results.txt inside a folder that starts with QA_RESULTS_* and not OUTPUT_*
Pls clarify

sampath1117 · 2023-09-12T07:30:27Z

.gitignore

@@ -12,6 +12,8 @@ hip_build/
 OUTPUT_IMAGES*
 OUTPUT_PERFORMANCE*
 QA_RESULTS*
+OUTPUT_AUDIO*
+QA_AUDIO*


This also need not be added, i feel
we can have one folder QA_RESULTS which can be used for both image and audio

sampath1117 · 2023-09-12T07:30:56Z

README.md


-## Build & Install RPP 


Please revert this spacing change

sampath1117 · 2023-09-12T07:32:57Z

src/modules/cpu/host_tensor_audio_augmentations.hpp

+
+#include "kernel/non_silent_region_detection.hpp"
+
+#endif // HOST_TENSOR_AUDIO_AUGMENTATIONS_HPP


@r-abishek
Please let us know if one space is needed for all files in RPP as well
In MIVisionX this convention is followed. I am not sure whether are we following the same in RPP as well

For #include we still have no-space convention for RPP which is fine for now. Can do a global change PR if needed.

sampath1117 · 2023-09-12T07:36:38Z

utilities/test_suite/HOST/README_AUDIO.md

+``` python
+python runAudioTests.py --case_start 0 --case_end 0 --test_type 0 --qa_mode 1 --batch_size 8
+```
+-   Unit test mode - Unit tests allowing users to pass a path to a folder containing audio files, to execute the desired functionality and variant once, report RPP execution wall time, save and view output


Please remove the save and view output, since for audio, there is no output that is gonna be generated

sampath1117 · 2023-09-12T07:44:40Z

utilities/test_suite/HOST/runAudioTests.py

+        exit()
+
+# Check if the folder is the root folder or exists, and remove the specified subfolders
+def validate_and_remove_folders(path, folder):


For python also we have some common functions which can be used across test suites
May be we can have this common code in a file called commons.py and use it across?

Not necessarily now, but better we do it this way
@r-abishek kindly request you to share your thoughts on this

Agreed, keep similar names for the cpp and python common headers. Use the word test_suite or say testSuiteCommon.py
Please add this one to your running test suite list of tasks.

sampath1117 · 2023-09-12T07:45:47Z

utilities/test_suite/HOST/runAudioTests.py

+# Get a list of log files based on a flag for preserving output
+def get_log_file_list(preserveOutput):
+    return [
+        "../../OUTPUT_AUDIO_PERFORMANCE_LOGS_HOST_" + timestamp + "/Tensor_host_audio_raw_performance_log.txt",


Lets rename the file as OUTPUT_PERFORMANCE_AUDIO_XX, so that we need to add new formats in .gitignore for audio alone

sampath1117 · 2023-09-12T07:48:21Z

utilities/test_suite/HOST/runAudioTests.py

+qaMode = args.qa_mode
+preserveOutput = args.preserve_output
+batchSize = args.batch_size
+bitDepth = 2


Please add a comment to this stating
"bitdepth is fixed to 2 since only bitdepth 2 (F32) only supported for audio"

sampath1117 · 2023-09-12T07:48:55Z

utilities/test_suite/HOST/runAudioTests.py

+    if qaMode:
+        outFilePath = os.path.join(os.path.dirname(cwd), 'QA_AUDIO_RESULTS_HOST_' + timestamp)
+    else:
+        outFilePath = os.path.join(os.path.dirname(cwd), 'OUTPUT_AUDIO_HOST_' + timestamp)


Please remove the usage of this OUTPUT_AUDIO_HOST

discussed about this change and decided to have the folder name as OUTPUT_AUDIO_HOST for now

sampath1117 · 2023-09-12T07:49:24Z

utilities/test_suite/HOST/runAudioTests.py

+    numRuns = 1
+elif(testType == 1):
+    if "--num_runs" not in sys.argv:
+        numRuns = 100#default numRuns for running performance tests


Add space after 100

snehaa8 · 2023-09-12T10:51:38Z

Confirmed NSR QA tests is unaffected after testsuite update.

r-abishek · 2023-09-20T03:12:01Z

utilities/test_suite/HOST/Tensor_host_audio.cpp

+
+    }
+
+


Please go over the code quickly and remove any double line spaces. Also from L263-L278, lets compact by removing too many empty lines - similar elsewhere

Done
Somehow it is not showing in github UI though

…nsor

fixed bug in ROI updation for audio test suite resolved issue in summary generation for performance tests in python

r-abishek · 2023-09-27T02:26:34Z

src/modules/cpu/kernel/non_silent_region_detection.hpp

+    return (value * value);
+}
+
+Rpp32f getMax(Rpp32f *values, Rpp32s srcLength)


Can you change this to use std::max_element(values, values + srcLength)? And remove this, please check output.

Done
Verified with QA tests and it passes

r-abishek · 2023-09-27T02:28:29Z

src/modules/cpu/kernel/non_silent_region_detection.hpp

+    {
+        Rpp32f *srcPtrTemp = srcPtr + batchCount * srcDescPtr->strides.nStride;
+        Rpp32s srcLength = srcLengthTensor[batchCount];
+        Rpp32f *mmsBuffer = static_cast<Rpp32f *>(calloc(srcLength, sizeof(Rpp32f)));


Please add a comment here stating why specifically srcLength could be a wide range, and its hard to statically initialize or pre-initialize during handle creation. You can even give a range

Done
Again somehow this change is not being shown in github ui

r-abishek · 2023-09-27T02:41:06Z

utilities/test_suite/HOST/Tensor_host_audio.cpp

+    // Set numDims, offset, n/c/h/w values for src/dst
+    srcDescPtr->numDims = 4;
+    srcDescPtr->offsetInBytes = 0;
+    srcDescPtr->n = batchSize;


Can't we use set_descriptor_dims_and_strides() like in Tensor_host.cpp?
Also, don't we have calls to these below?

// Set src/dst layout types in tensor descriptors set_descriptor_layout(srcDescPtr, dstDescPtr, layoutType, pln1OutTypeCase, outputFormatToggle); // Set src/dst data types in tensor descriptors set_descriptor_data_type(inputBitDepth, funcName, srcDescPtr, dstDescPtr);

For audio there is no input layout
So we are not able to use the functions defined for image in audio

Add few more helpers to modularize the code

r-abishek · 2023-09-27T02:44:11Z

utilities/test_suite/HOST/Tensor_host_audio.cpp

+                        missingFuncFlag = 1;
+
+                    if (testType == 0)
+                        verify_non_silent_region_detection(detectedIndex, detectionLength, testCaseName, batchSize, audioNames, dst);


The verify_non_silent_region_detection() can't be included inside the time taken to run the function. @sampath1117 this Audio test suite file needs some cleanup, before external PR. Please check.

Is it ok if i declare some static buffers for begin and length outside the switch case and have a branch statement in the end of switch case calling two different verify functions based on the case?

Please let me know your thoughts

Added a comment stating it won't run in performance tests

snehaa8 added 4 commits August 3, 2023 11:10

Initial commit - Non slient region detection

6627464

Includes unittest setup

Replace vectors with arrays

3720c8f

Replace Rpp64s with Rpp32s

f7f51c8

Optimize and precompute cutOff

676de7c

r-abishek assigned snehaa8 Aug 31, 2023

r-abishek added the enhancement New feature or request label Aug 31, 2023

r-abishek added this to the sow9ms4 milestone Aug 31, 2023

r-abishek requested changes Aug 31, 2023

View reviewed changes

sampath1117 reviewed Sep 1, 2023

View reviewed changes

snehaa8 added 3 commits September 4, 2023 04:17

Fix buffer used

46b8fb5

Fix buffer used

11fb3ab

Update testsuite for Audio

79d9e6f

r-abishek reviewed Sep 11, 2023

View reviewed changes

HazarathKumarM added 2 commits September 12, 2023 02:47

code cleanup

5719db9

Add Readme file for Audio test suite

4aad4d6

sampath1117 reviewed Sep 12, 2023

View reviewed changes

HazarathKumarM added 2 commits September 12, 2023 05:52

changes based on review comments

abf15d8

minor change

71ef2f5

HazarathKumarM added 3 commits September 14, 2023 03:54

Remove unittest folders and updated README.md

adad92b

Remove unit tests

8e01e0e

minor change

69fba3e

r-abishek reviewed Sep 20, 2023

View reviewed changes

sampath1117 added 5 commits September 20, 2023 06:57

code cleanup

a7b1b22

added common header file for audio helper functions

441bfac

Merge remote-tracking branch 'abishek_rpp/master' into sn/nsr_host_te…

c8803c2

…nsor

removed unncessary audio wav files

e7169fe

fixed bug in ROI updation for audio test suite resolved issue in summary generation for performance tests in python

removed log file

11b4709

sampath1117 added 2 commits September 21, 2023 12:12

added doxygen support for audio

d64bc7b

minor change

223de61

r-abishek reviewed Sep 27, 2023

View reviewed changes

sampath1117 added 3 commits September 27, 2023 06:12

removed the usage of getMax function and used std::max_element

512a79f

modularized code in test suite

6a18957

minor change

2247696

r-abishek approved these changes Sep 27, 2023

View reviewed changes

r-abishek changed the base branch from master to ar/audio_support_1_non_silent_region September 27, 2023 15:19

r-abishek merged commit 87b0138 into r-abishek:ar/audio_support_1_non_silent_region Sep 27, 2023

r-abishek mentioned this pull request Aug 16, 2024

Audio support in RPP - HOST #78

Closed


		#include "kernel/non_silent_region_detection.hpp"

		#endif // HOST_TENSOR_AUDIO_AUGMENTATIONS_HPP

PR1 - Audio - Non silent region detection HOST Tensor #149

PR1 - Audio - Non silent region detection HOST Tensor #149

Conversation

snehaa8 commented Aug 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sampath1117 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snehaa8 commented Sep 8, 2023

r-abishek commented Sep 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sampath1117 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snehaa8 commented Sep 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment