Javascript audio player for the exported HTML file.

Javascript audio player for the exported HTML file.

BUG=webrtc:7218
NOTRY=True

Review-Url: https://codereview.webrtc.org/2721023002
Cr-Commit-Position: refs/heads/master@{#17529}
Committed: https://chromium.googlesource.com/external/webrtc/+/fbd4f85f2dab93d819e42d1b1dfc6e2b2125ddf2

[AleBzk, 2017-02-28 16:07:30]

Description was changed from

==========
Javascript audio player for the exported HTML file.

BUG=webrtc:7218
NOTRY=True
==========

to

==========
Javascript audio player for the exported HTML file.

BUG=webrtc:7218
NOTRY=True
==========

[AleBzk, 2017-02-28 16:07:31]

alessiob@webrtc.org changed reviewers:
+ jansson@webrtc.org

[AleBzk, 2017-02-28 16:36:31]

Hi Christoffer,

This CL is part of the APM Quality Assessment tool, which is implemented in
Python and tests the APM module under different conditions (flags, probing
signals, noise sources). The generated scores can be exported in an HTML file.
To better understand the values, I added a simple audio player with which one
can listen to the input track, the output from the APM module, and the reference
track (that is the signal used to compute scores).

[janssonWebRTC, 2017-03-02 10:19:32]

jansson@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[janssonWebRTC, 2017-03-02 10:19:34]

LGTM with style nits, looks like the linter is very lenient. Adding kjellander@
as well due to owners (not sure if I have enough rights)

https://codereview.webrtc.org/2721023002/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.css
(right):

https://codereview.webrtc.org/2721023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.css:62:
.inspector{
Declarations should be sorted in alphabetical order.
https://google.github.io/styleguide/htmlcssguide.html#Declaration_Order

https://codereview.webrtc.org/2721023002/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.js
(right):

https://codereview.webrtc.org/2721023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.js:14:
this._audio_player = new Audio();
The snake_case looks like a carry over from python, for javascript use
camelCase. HTML elements you can use either dash-form or camelCase. This applies
to the whole file.

https://www.chromium.org/developers/web-development-style-guide

https://codereview.webrtc.org/2721023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.js:43:
console.debug('number of "score" items: ' + items.length);
This is the only place where you have used the console, is this a leftover from
debugging or do you need it?

https://codereview.webrtc.org/2721023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.js:55:
case 49:
Since there are so few key bindings, maybe include a comment of which actual
keyboard key they represent?

https://codereview.webrtc.org/2721023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.js:67:
break;
Unnecessary break.

https://codereview.webrtc.org/2721023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.js:102:
Inspector.prototype.play_audio_in = function(url) {
url is unused.

https://codereview.webrtc.org/2721023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.js:110:
Inspector.prototype.play_audio_out = function(url) {
url is unused.

https://codereview.webrtc.org/2721023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.js:118:
Inspector.prototype.play_audio_ref = function(url) {
url is unused.

https://codereview.webrtc.org/2721023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.js:135:
alert('Select a score firts.');
firts > first

https://codereview.webrtc.org/2721023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.js:150:
this._inspector_node.innerHTML = '<div class="property noise-generator">' +
Maybe you could use document.createElement() here as well as you have done in
the function Inspector() constructor and then add them as child nodes?

[AleBzk, 2017-03-02 11:06:12]

Hi Christoffer, thanks a lot for your comments.
I was under Python influence, sorry for the wrong naming.

https://codereview.webrtc.org/2721023002/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.css
(right):

https://codereview.webrtc.org/2721023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.css:62:
.inspector{
On 2017/03/02 10:19:32, janssonWebRTC wrote:
> Declarations should be sorted in alphabetical order.
> https://google.github.io/styleguide/htmlcssguide.html#Declaration_Order

Done.

https://codereview.webrtc.org/2721023002/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.js
(right):

https://codereview.webrtc.org/2721023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.js:14:
this._audio_player = new Audio();
On 2017/03/02 10:19:33, janssonWebRTC wrote:
> The snake_case looks like a carry over from python, for javascript use
> camelCase. HTML elements you can use either dash-form or camelCase. This
applies
> to the whole file.
> 
> https://www.chromium.org/developers/web-development-style-guide

Done.

https://codereview.webrtc.org/2721023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.js:14:
this._audio_player = new Audio();
On 2017/03/02 10:19:33, janssonWebRTC wrote:
> The snake_case looks like a carry over from python, for javascript use
> camelCase. HTML elements you can use either dash-form or camelCase. This
applies
> to the whole file.
> 
> https://www.chromium.org/developers/web-development-style-guide

Done.

https://codereview.webrtc.org/2721023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.js:43:
console.debug('number of "score" items: ' + items.length);
On 2017/03/02 10:19:33, janssonWebRTC wrote:
> This is the only place where you have used the console, is this a leftover
from
> debugging or do you need it?

Thanks, it's the second :)
I'll remove the line.

https://codereview.webrtc.org/2721023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.js:43:
console.debug('number of "score" items: ' + items.length);
On 2017/03/02 10:19:33, janssonWebRTC wrote:
> This is the only place where you have used the console, is this a leftover
from
> debugging or do you need it?

Done.

https://codereview.webrtc.org/2721023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.js:55:
case 49:
On 2017/03/02 10:19:33, janssonWebRTC wrote:
> Since there are so few key bindings, maybe include a comment of which actual
> keyboard key they represent?

Done.

https://codereview.webrtc.org/2721023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.js:67:
break;
On 2017/03/02 10:19:33, janssonWebRTC wrote:
> Unnecessary break.

True, but isn't it safer to keep it?
If someone adds a case entry below, he/she would probably need to add break.

https://codereview.webrtc.org/2721023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.js:102:
Inspector.prototype.play_audio_in = function(url) {
On 2017/03/02 10:19:33, janssonWebRTC wrote:
> url is unused.

Done.

https://codereview.webrtc.org/2721023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.js:110:
Inspector.prototype.play_audio_out = function(url) {
On 2017/03/02 10:19:33, janssonWebRTC wrote:
> url is unused.

Done.

https://codereview.webrtc.org/2721023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.js:118:
Inspector.prototype.play_audio_ref = function(url) {
On 2017/03/02 10:19:32, janssonWebRTC wrote:
> url is unused.

Done.

https://codereview.webrtc.org/2721023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.js:135:
alert('Select a score firts.');
On 2017/03/02 10:19:33, janssonWebRTC wrote:
> firts > first

Done.

https://codereview.webrtc.org/2721023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.js:150:
this._inspector_node.innerHTML = '<div class="property noise-generator">' +
On 2017/03/02 10:19:33, janssonWebRTC wrote:
> Maybe you could use document.createElement() here as well as you have done in
> the function Inspector() constructor and then add them as child nodes?

Thanks for your comment.
I used createElement() for those DOM nodes I need to access.
In this case, it's better to use innerHTML to reduce boilerplate code and make
the code more readable.

[janssonWebRTC, 2017-03-02 12:03:20]

https://codereview.webrtc.org/2721023002/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.js
(right):

https://codereview.webrtc.org/2721023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.js:67:
break;
On 2017/03/02 11:06:11, AleBzk wrote:
> On 2017/03/02 10:19:33, janssonWebRTC wrote:
> > Unnecessary break.
> 
> True, but isn't it safer to keep it?
> If someone adds a case entry below, he/she would probably need to add break.

That's true, will leave it up to you.

https://codereview.webrtc.org/2721023002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.js:150:
this._inspector_node.innerHTML = '<div class="property noise-generator">' +
On 2017/03/02 11:06:11, AleBzk wrote:
> On 2017/03/02 10:19:33, janssonWebRTC wrote:
> > Maybe you could use document.createElement() here as well as you have done
in
> > the function Inspector() constructor and then add them as child nodes?
> 
> Thanks for your comment.
> I used createElement() for those DOM nodes I need to access.
> In this case, it's better to use innerHTML to reduce boilerplate code and make
> the code more readable.
Ah right, I get your point.

[kjellander_webrtc, 2017-03-03 04:58:40]

lgtm but I'm also not owner. My comment about considering jinja2 templating
library in https://codereview.webrtc.org/2723703002/ also applies here.

https://codereview.webrtc.org/2721023002/diff/20001/webrtc/modules/audio_proc...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.js
(right):

https://codereview.webrtc.org/2721023002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/results.js:148:
'</div>' +
-1 indent on line 147-152

[AleBzk, 2017-04-04 13:56:58]

The CQ bit was checked by alessiob@webrtc.org

[AleBzk, 2017-04-04 13:56:58]

The patchset sent to the CQ was uploaded after l-g-t-m from jansson@webrtc.org,
kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2721023002/#ps40001 (title:
"rebase")

[commit-bot: I haz the power, 2017-04-04 13:57:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-04-04 13:59:16]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491314218484380,
"parent_rev": "fab67071a4ee3411482cd3a52eae22924b4d8b18", "commit_rev":
"fbd4f85f2dab93d819e42d1b1dfc6e2b2125ddf2"}

[commit-bot: I haz the power, 2017-04-04 13:59:19]

Description was changed from

==========
Javascript audio player for the exported HTML file.

BUG=webrtc:7218
NOTRY=True
==========

to

==========
Javascript audio player for the exported HTML file.

BUG=webrtc:7218
NOTRY=True

Review-Url: https://codereview.webrtc.org/2721023002
Cr-Commit-Position: refs/heads/master@{#17529}
Committed:
https://chromium.googlesource.com/external/webrtc/+/fbd4f85f2dab93d819e42d1b1...
==========

[commit-bot: I haz the power, 2017-04-04 13:59:20]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/fbd4f85f2dab93d819e42d1b1...