Open Bug 964498 Opened 10 years ago Updated 2 years ago

Add SNR measurements to Talos for audio tests.

Categories

(Core :: WebRTC: Audio/Video, defect, P3)

x86
Linux
defect

Tracking

()

REOPENED
mozilla34

People

(Reporter: snandaku, Unassigned)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-needed, Whiteboard: [needs documentation])

Attachments

(12 files, 8 obsolete files)

5.59 KB, patch
jesup
: review+
jmaher
: review-
Details | Diff | Splinter Review
5.27 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
10.61 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
198 bytes, text/plain
jmaher
: feedback-
Details
36.29 KB, patch
dminor
: review+
Details | Diff | Splinter Review
903 bytes, patch
kmoir
: review+
Details | Diff | Splinter Review
40.79 KB, patch
Details | Diff | Splinter Review
40.79 KB, patch
Details | Diff | Splinter Review
3.22 KB, patch
Details | Diff | Splinter Review
49.47 KB, text/plain
Details
5.73 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
45.35 KB, text/plain
Details
Idea is to add SNR computation tool to Talos for existing media tests and thus replacing PESQ tool till we figure out Licensing issues.
Blocks: 906942
Assignee: nobody → snandaku
Attached file MediaTools Framework
Attachment #8366466 - Flags: feedback?(jmaher)
Attachment #8366462 - Flags: review?(rjesup)
Attachment #8366462 - Flags: review?(jmaher)
Attachment #8366463 - Flags: review?(jmaher)
Attachment #8366464 - Flags: review?(jmaher)
Attachment #8366462 - Attachment description: Add SNR tool support to python tools → Add SNR tool invocation to Talos
OS: Mac OS X → Linux
Comment on attachment 8366463 [details] [diff] [review]
Update JS tests to invoke SNR computation

Review of attachment 8366463 [details] [diff] [review]:
-----------------------------------------------------------------

I really don't understand the difference between PESQ and SNR.  From the implementation details of this patch it looks like a straight replacement with a test name change involved (i.e. we need to update the database).

::: talos/startup_test/media/html/media_api.js
@@ +75,5 @@
>  };
>  
> +// Perform audio/snr/compute command
> +var getSNRAndDelay = function(test) {
> +  var url = baseUrl + '/audio/snr/compute';

I assume this url is defined somewhere?
Attachment #8366463 - Flags: review?(jmaher) → review+
Comment on attachment 8366462 [details] [diff] [review]
Add SNR tool invocation to Talos

Review of attachment 8366462 [details] [diff] [review]:
-----------------------------------------------------------------

::: talos/startup_test/media/media_utils.py
@@ +28,5 @@
>  """
>  _TOOLS_PATH_             = os.path.join(here,'tools')
>  _TEST_PATH_              = os.path.join(here,'html')
>  _PESQ_                   = os.path.join(_TOOLS_PATH_, 'PESQ')
> +_MEDIA_TOOLS_            = os.path.join(_TOOLS_PATH_, 'MediaUtils')

i assume we have another patch to define this binary?

@@ +175,5 @@
> +               _RECORDED_NO_SILENCE_]
> +        cmd =  [str(s) for s in cmd]
> +
> +        output = subprocess.check_output(cmd)
> +        #SNR_Delay=1.063,5

why is this here?  Is this a comment or debugging code?  please remove or add more details.

@@ +191,5 @@
> +            We return status as True since SNR computation went through
> +            successfully but scores computation failed due to severly
> +            degraded audio quality.
> +            """
> +            return True, snr_delay

we return the same thing in both cases of the if/else, we can clean that up
Attachment #8366462 - Flags: review?(jmaher) → review-
Comment on attachment 8366464 [details] [diff] [review]
Add SNR Binary to Talos

Review of attachment 8366464 [details] [diff] [review]:
-----------------------------------------------------------------

I assume this will work for linux32 and linux64, is this marked as executable?
Attachment #8366464 - Flags: review?(jmaher) → review+
Comment on attachment 8366466 [details]
MediaTools Framework

Are there other places that use this toolchain, or only talos?

We don't have any code in talos that requires building, only binary versions.  For example latency-benchmark (https://github.com/google/latency-benchmark), produces binary version and we consume those.  Would it be ok to do that process?
Attachment #8366466 - Flags: feedback?(jmaher) → feedback-
Hi Randell

   What do you suggest be a good place to host the native tools we build/develop as part of the Talos media testing framework (this is in response to :jmaher's response in Commnet 8). As of today, the SNR code is hosted on my git-repo at https://github.com/suhasHere/MediaPerf/tree/master/media-tools.

  I am presuming we will be adding more native tools for , say, PSNR, SSIM and probably other audio quality tools over the time. We need to ensure that the native code can be reviewed and then Talos can use the pre-built binaries where needed.

Any thoughts on this is welcome

Thanks
Flags: needinfo?(rjesup)
On discussion with Randell, we decided that review for the native code will be done in the git repo for now and Talos can use the pre-built binary. 
Opened a bug to track the same: Bug 965623
Flags: needinfo?(rjesup)
Comment on attachment 8366462 [details] [diff] [review]
Add SNR tool invocation to Talos

Review of attachment 8366462 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with jmaher's issues addressed
Attachment #8366462 - Flags: review?(rjesup) → review+
are we in a position to use this tool and start running tests?  It would probably take 2 weeks to sort out on my end, I have been waiting on this before doing the final deployment steps.
Assignee: snandaku → rjesup
Assignee: rjesup → jib
I finally got this running on try server!

My changes to talos are up here:
http://hg.mozilla.org/users/jmaher_mozilla.com/tpain (this has a lot of other changes as well)

Next steps:
* [jmaher] - get a diff patch from what is already checked in
* [jmaher] - get test name added to graph server, and update the databases
* [jmaher] - land changes
* [jmaher] - run on try, verify stable numbers 
* [jib/??] - add documentation for the test to https://wiki.mozilla.org/Buildbot/Talos/Tests
* [jib/??] - determine if higher is better/worse - help document what to do in a regression
* [jmaher/releng] - get a change for linux64 buildbot to add this to a talos job
* [jmaher/releng] - deploy latest changes for buildbot and talos.json
* [all] - party
Assignee: jib → jmaher
do these tests need to wait for mozafterpaint to load?  a lot of our tests require mozafterpaint event before doing work or taking measurements- I am not sure if this is one of them.
Depends on: 1000467
Thanks, Joel, for helping make this happen.  Any idea how long until we can "party"?  :-)

Also (re: Comment 14) these tests shouldn't need to wait for mozpaint to load.
I want to get most of this done this week.  The buildbot stuff will be next week and ironically I am at a releng work week- so I will be in the right room with the right people to make that happen.

By May 2nd (end of next week), we could probably be ready to go live if we haven't already.
Blocks: 970426
Depends on: 1000792
done:
* [jmaher] - get test name added to graph server, and update the databases

ok, I have ran into instability in the tests- I believe it is an error on my end.  A small hiccup, but when it does run, the numbers look stable- a little too stable.
Priority: -- → P2
Whiteboard: [p=2, 1.5:p2, ft:webrtc]
Target Milestone: --- → mozilla32
We want to complete Bug 965623 in Fx32, but it's not blocking the work in this bug.
No longer depends on: 965623
I have been looking into this- in fact I get crashes during the shutdown of the browser:
https://tbpl.mozilla.org/php/getParsedLog.php?id=39152301&tree=Try&full=1

I didn't see this locally as I wasn't looking for minidumps.  Ignoring the minidumps I get 2 out of 3 runs to pass green.  This is something I need to look at further.  The test is running 5 times to reduce noise and 

Can I get someone to look into this crash?  It is very easy to reproduce on a loaner slave if you need me to try anything out.
Hey Jan-Ivar, Can you help Joel by looking into the crash he's reporting (Comment 19)?   Please let us know how difficult it is to reproduce and how tough it seems to fix.  If it starts looking hairy, I can get you help on this or some of the other bugs assigned to you for Fx 32.  Thanks!
Flags: needinfo?(jib)
Will do.
Depends on: 1007194
Flags: needinfo?(jib)
any luck on debugging this?
Depends on: 1013975
After hacking on over 1000 jobs on try server, it appears the method we were using to post (xmlhttprequest -> post) the results from the browser to a webserver was causing problems.  Removing the posting and dumping the output of the results to the console we can now get the results inside the talos harness and not have random timeouts.

What the exact problem is, I don't know- it would be much nicer and cleaner to have figured this out and retain the model of posting to an arbitrary server.  It appears to be the act of posting which is problematic, although I have reworked this to remove a layer of process indirection and management of the browser/webserver to the main talos process.

This patch removes a lot of the PESQ code, webserver code, and process management code.  The intention is for this to be cleaner and easier to work with going forward.  Please be critical in reviewing this.

Next steps:
* figure out the stability of the numbers (they fluctuate too much)
* ensure they stay stable going forward
* do our magic buildbot stuff
* determine how many iterations to run this for useful/reliable numbers
Attachment #8432693 - Flags: review?(dminor)
Comment on attachment 8432693 [details] [diff] [review]
update media tests to work reliably in talos (1.0)

Review of attachment 8432693 [details] [diff] [review]:
-----------------------------------------------------------------

There are a few things to be cleaned up prior to landing this, but for the most part this looks good.

::: talos/PerfConfigurator.py
@@ +447,5 @@
>          if self.config.get('develop'):
>              if not self.config.get('webserver'):
>                  s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
> +                s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
> +                s.bind(("127.0.0.1",15707))

Why are we moving to a fixed port number here?

::: talos/startup_test/media/html/media_api.js
@@ +37,4 @@
>  var sendTalosResults = function(talos_url, result_obj) {
>    var request = new XMLHttpRequest();
>    request.open('GET', talos_url, true);
> +  request.send();

Is this function still present to indicate to the server the tests are complete?

::: talos/startup_test/media/html/pc_audio_quality.js
@@ +11,3 @@
>  // audio from an input file into the Peer Connection.
>  // Once played out, the audio at the speaker is recorded to compute
>  // PESQ scores

nit: Should be SNR

::: talos/startup_test/media/media_manager.py
@@ +132,4 @@
>      talos.utils.info("Server %s at %s:%s" ,
>        httpd_server.docroot, httpd_server.host, httpd_server.port)
>      ObjectDb.httpd_server = httpd_server
> +    httpd_server.start()

Why don't we have block=True anymore?

::: talos/ttest.py
@@ +416,5 @@
> +                    #TODO: make this into a better setup function
> +                    if test_config['name'] == 'media_tests':
> +                        from startup_test.media import media_manager
> +                        mm_httpd = media_manager.run_server(os.path.dirname(os.path.realpath(__file__)))
> +

If there is anything to fix, now is the time. Otherwise, you should remove the comment.

@@ +437,5 @@
>   
> +                    #TODO: make this into a better cleanup location
> +                    if test_config['name'] == 'media_tests' and mm_httpd:
> +                        mm_httpd.stop()
> +

Please fix or remove the TODO comment.

@@ +454,5 @@
> +                # For startup tests, we launch the browser multiple times with the same profile
> +                try:
> +                    os.remove(os.path.join(profile_dir, 'sessionstore.js'))
> +                    os.remove(os.path.join(profile_dir, '.parentlock'))
> +                    os.remove(os.path.join(profile_dir, 'sessionstore.bak'))

Are the either all present or all missing? Otherwise, if one of these throws, the following removes won't occur.
Attachment #8432693 - Flags: review?(dminor) → review-
Updated patch with review comments addressed.  Thanks for the details.

There will be another patch in the near future where we need to do some hacking on the results so they can be reported and tracked properly!
Attachment #8432693 - Attachment is obsolete: true
Attachment #8433509 - Flags: review?(dminor)
Comment on attachment 8433509 [details] [diff] [review]
allow for stable media tests to run inside of talos on infrastructure (2.0)

Review of attachment 8433509 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM
Attachment #8433509 - Flags: review?(dminor) → review+
landed on talos:
https://hg.mozilla.org/build/talos/rev/d4ab382f733e

Still more work for full deployment, I am going to verify final version on try server:
https://tbpl.mozilla.org/?tree=Try&rev=27317475a3bd

remaining items:
* buildbot-custom config to run tests on linux64 only
* deploy buildbot changes
* land talos.json changes
* some time fix the silence/delay
* some time change back to PESQ
this will require us to land a talos.json that supports other_nolinux64 and other_linux64
Attachment #8435997 - Flags: review?(pmoore)
Comment on attachment 8435997 [details] [diff] [review]
support linux64 custom test suite in talos (1.0)

Hey Joel,

I'm on project duty this week and next week, and then on holiday the week after. I don't suppose you could find someone else to review this one? Otherwise I can have a look in three weeks, if it isn't urgent.

Sorry about that!

Thanks,
Pete
Attachment #8435997 - Flags: review?(pmoore)
Attachment #8435997 - Flags: review?(kmoir)
Comment on attachment 8435997 [details] [diff] [review]
support linux64 custom test suite in talos (1.0)

This patch doesn't apply cleanly but if I fixed it on my dev-master.  I'll attach to builder diff so you can see what the changes look like. So we need a new patch.
Attached file bug964498builder.diff (obsolete) —
builder diff
thanks kmoir for the builder diff.  Looking at this, I see no surprises.  We could have a potential conflict on try if we push an older version to try and run talos (rare, but it happens).  In this case we would need to not run the newer suites (other_nolinux64, other_linux64) and only run the suite 'other'.  I don't know a clean solution for this.
jmaher you may have misinterpreted my earlier comment.  I meant that the patch didn't apply cleanly to the repo, not that the builder names were a problem.
updated to fix bitrot!
Attachment #8435997 - Attachment is obsolete: true
Attachment #8435997 - Flags: review?(kmoir)
Attachment #8437191 - Flags: review?(kmoir)
Comment on attachment 8437191 [details] [diff] [review]
support linux64 custom test suite in talos (1.1)

This looks good, just as a fyi I think aki is doing mergeduty right now so he doesn't want new patches landed.
Attachment #8437191 - Flags: review?(kmoir) → review+
I will look to land this later this week when the uplift is good and done.  I need to land some patches on trunk (and now aurora) to have these test targets defined in talos.josn first
this is required to be on firefox 32+ before we land the buildbot-configs changes (well technically do the reconfig after landing).  we can land these with dontbuild/npotb as there are no real changes, only support for future changes.
Attachment #8437694 - Flags: review?(kmoir)
Attachment #8437694 - Flags: review?(kmoir) → review+
aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/7a88df99c39c

we need a day for trunk to get all the changes, then we can do the buildbot work.

:kmoir, should we land the buildbot changes today and wait for a reconfig tomorrow?
Flags: needinfo?(kmoir)
I think it's fine to land now
Flags: needinfo?(kmoir)
Blocks: 1023648
No longer blocks: 970426
https://hg.mozilla.org/mozilla-central/rev/8a2afe947868
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: mozilla32 → mozilla33
landed buildbot-configs change:
https://hg.mozilla.org/build/buildbot-configs/rev/60330473c522

this will get picked up automatically when we do a reconfig in the near future.

We need documentation for the test on the wiki:
https://wiki.mozilla.org/Buildbot/Talos/Tests

:jesup, can you do that or find someone to do it?
Flags: needinfo?(rjesup)
Live with reconfig on 2014-06-11 08:47 PT
Depends on: 1023952
Attachment #8437191 - Flags: checkin-
Backed out in revision 31fb3ec3f3ac as requested by RyanVM since it was causing perma-fail on tbpl.
No longer depends on: 1023952
Depends on: 1023952
Back out commit live with reconfig on 2014-06-11 10:16 PT
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
round 2!
Attachment #8437191 - Attachment is obsolete: true
Attachment #8439983 - Flags: review?(kmoir)
Attached file bug8439983builder.diff (obsolete) —
Here's the builder diff, is this what you expect?
this isn't what I would expect :(  some reason svgr is removed and added from the list, seems ok.

the graphics, ux, elm, holly, and oak branches are just adding other_nol64 for osx 10.6/10.8, Win 8/7/XP, odd we are not removing the other test.

For Ubuntu* we are removing other, but not adding other_l64, I am not sure why after looking at my patch.
updated the patch to get the linux64 only platform a different way.  I also have a need for a new suite/job for talos and am sneaking that in here.  It will run on all platforms.
Attachment #8439983 - Attachment is obsolete: true
Attachment #8439983 - Flags: review?(kmoir)
Attachment #8442337 - Flags: review?(kmoir)
new builder diff for reference
Attachment #8436944 - Attachment is obsolete: true
Attachment #8440727 - Attachment is obsolete: true
ah, I forgot to turn on the g1 suite as well, luckily the linux64 stuff was working as expected.
Attachment #8442337 - Attachment is obsolete: true
Attachment #8442337 - Flags: review?(kmoir)
Attachment #8443041 - Flags: review?(kmoir)
Kim, thanks for uploading the builder diffs.  The l64/no_l64 stuff is great.  I found an error in my patch which didn't add the g1 tests properly, that should be fixed.  At your convenience, could you rerun the builder diff?
Attachment #8443041 - Attachment is obsolete: true
Attachment #8443041 - Flags: review?(kmoir)
Attachment #8443527 - Flags: review?(kmoir)
Attached file new builder diff
this does what I want it to do, the only odd thing here is these branches we seem to be adding new tests, but not removing tests:
elm, oak, holly, graphics, ux

could it be that these branches have no talos on and now we would be adding it?
Depends on: 1028999
so in config.py (the file I am editing), oak, graphics, ux are not mentioned specifically- elm and holly are, although I am not clear if they are doing anything specific to talos tests.

:kmoir, can you help out here?  we might just need to understand this and all is well.
Flags: needinfo?(kmoir)
Attachment #8443527 - Flags: review?(kmoir) → review+
Flags: needinfo?(kmoir)
Some of the project branches have Talos disabled.  They include aok, elm, holly and ux.   You can see this in project_branches.py
Attachment #8443527 - Flags: review+ → review?(kmoir)
Attached patch bug964498.patchSplinter Review
This patch fixes the issue with the project branches getting talos enabled.  I tested it on my dev-master and will attach the builder diff
Attachment #8445191 - Flags: review?(jmaher)
Attached file bug964498builder.diff
new builder diff
Comment on attachment 8445191 [details] [diff] [review]
bug964498.patch

Review of attachment 8445191 [details] [diff] [review]:
-----------------------------------------------------------------

this looks like the same as my patch with the magic line to check for enable_talos :)
Attachment #8445191 - Flags: review?(jmaher) → review+
ok, the builderdiff looks great.  I will need to land the talos.json changes to aurora and inbound- inbound has been closed for a while, so in the waiting queue.  Once that is landed we can "reconfig"
Comment on attachment 8445191 [details] [diff] [review]
bug964498.patch

jmaher: Yes, I had the corrected patch on my dev-master so I thought the easiest thing would be to attach it :-)
Attachment #8443527 - Flags: review?(kmoir)
heads up, I will land this patch on Tuesday and be around for the next 3 days- I assume we will do a reconfig sometime then in case I am needed.
landed buildbot config changes:
https://hg.mozilla.org/build/buildbot-configs/rev/7e17f8e5484c

Jesup, please ensure you take care of the documentation for this test this week:
https://wiki.mozilla.org/Buildbot/Talos/Tests#media_tests
Depends on: 1033323
Target Milestone: mozilla33 → mozilla34
these have been on since firefox 32 (about 3 weeks after it went to Aurora).  So we have the talos media_tests on Beta, Aurora and Trunk right now.  We still need documentation written about this, otherwise all questions (instead of most) will need to be answered by someone on the webrtc team.

here is what needs to be updated:
https://wiki.mozilla.org/Buildbot/Talos/Tests#media_tests
Assignee: jmaher → rjesup
Whiteboard: [p=2, 1.5:p2, ft:webrtc] → [p=1, ft:webrtc, needs docuementation]
Priority: P2 → P1
Depends on: 1052467
4 months later and we still have no documentation on this test: https://wiki.mozilla.org/Buildbot/Talos/Tests#media_tests.  More importantly after seeing this in action for a few months, we have very noisy data (multi-modal):
http://graphs.mozilla.org/graph.html#tests=%5B%5B317,63,35%5D,%5B317,131,35%5D%5D&sel=1407247807827,1415023807827&displayrange=90&datatype=running

:rjesup, can you look into this and figure out why we have this multi modal data?  I don't see this test providing much value.  We have not seen any regressions on these tests since they were turned on and that doesn't surprise me given the noisy looking graphs.  If there is value in keeping these tests please document them, I am inclined to turn them off.
Flags: needinfo?(rjesup)
backlog: --- → webRTC+
Rank: 15
Whiteboard: [p=1, ft:webrtc, needs docuementation] → [needs documentation]
Assignee: rjesup → nobody
Rank: 15 → 27
Priority: P1 → P2
Mass change P2->P3 to align with new Mozilla triage process.
Priority: P2 → P3
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: