Adding some audio debugging flags

Adding some flags to make audio debugging easier. Now we can separate audible audio from inaudible, and dump the tokens in their original encoded form.

rkc@ for copresence review.
andrew@webrtc for wav_file review.

Committed: https://crrev.com/4f0acbe307357802844fbb6e974ce724dea45655
Cr-Commit-Position: refs/heads/master@{#314772}

[Charlie, 2015-02-04 23:07:04]

ckehoe@chromium.org changed reviewers:
+ andrew@webrtc.org, rkc@google.com

[ajm, 2015-02-04 23:24:25]

ajm@chromium.org changed reviewers:
+ ajm@chromium.org

[ajm, 2015-02-04 23:24:26]

Just to be sure you're aware, there is some wav handling functionality in
Chromium proper:
https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/sounds...

which you might prefer to avoid a third_party dependency. Let me know what you
think.

[rkc, 2015-02-04 23:37:17]

rkc@chromium.org changed reviewers:
+ rkc@chromium.org

[rkc, 2015-02-04 23:37:18]

I'll echo what ajm@ said. Consider using the WavWriter in Chromium. We should
avoid adding third party dependencies where possible.

https://codereview.chromium.org/892423002/diff/20001/components/copresence/co...
File components/copresence/copresence_switches.h (right):

https://codereview.chromium.org/892423002/diff/20001/components/copresence/co...
components/copresence/copresence_switches.h:12: extern const char
kCopresenceDumpTokensToDir[];
This is a lot of flags - can some of this be moved into the debug UI as check
boxes? For example, enabling/disabling players seems like something that belongs
the UI much more than a flag.

https://codereview.chromium.org/892423002/diff/20001/components/copresence/me...
File components/copresence/mediums/audio/audio_manager_impl.cc (right):

https://codereview.chromium.org/892423002/diff/20001/components/copresence/me...
components/copresence/mediums/audio/audio_manager_impl.cc:311: DCHECK_EQ((int)
int_samples.size(), samples->frames());
How could this ever fail?

https://codereview.chromium.org/892423002/diff/20001/components/copresence/me...
components/copresence/mediums/audio/audio_manager_impl.cc:318: 1);
It isn't evident what this number means. Add a comment or make it a constant.

[Charlie, 2015-02-04 23:41:02]

On 2015/02/04 23:24:26, ajm wrote:
> Just to be sure you're aware, there is some wav handling functionality in
> Chromium proper:
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/sounds...
> 
> which you might prefer to avoid a third_party dependency. Let me know what you
> think.

Isn't that just for input from a wav file? I'm trying to output to a wav file.

[Charlie, 2015-02-04 23:52:14]

https://codereview.chromium.org/892423002/diff/20001/components/copresence/co...
File components/copresence/copresence_switches.h (right):

https://codereview.chromium.org/892423002/diff/20001/components/copresence/co...
components/copresence/copresence_switches.h:12: extern const char
kCopresenceDumpTokensToDir[];
On 2015/02/04 23:37:18, Rahul Chaturvedi wrote:
> This is a lot of flags - can some of this be moved into the debug UI as check
> boxes? For example, enabling/disabling players seems like something that
belongs
> the UI much more than a flag.
> 

There are two reasons I don't want to do this yet:

1. Flags are better for testing scripts. Without them, we also need some kind of
API so a script can toggle this switch.

2. Runtime support, in the case of either a UI or API, is more complex, because
the value can change during execution. We'd have to enable and disable playback
accordingly.

But I agree that this is the correct eventual solution. See the TODO in the .cc
file.

https://codereview.chromium.org/892423002/diff/20001/components/copresence/me...
File components/copresence/mediums/audio/audio_manager_impl.cc (right):

https://codereview.chromium.org/892423002/diff/20001/components/copresence/me...
components/copresence/mediums/audio/audio_manager_impl.cc:311: DCHECK_EQ((int)
int_samples.size(), samples->frames());
On 2015/02/04 23:37:18, Rahul Chaturvedi wrote:
> How could this ever fail?

Someone could edit the code incorrectly. One of the purposes of DCHECK is to
document our assumptions.

https://codereview.chromium.org/892423002/diff/20001/components/copresence/me...
components/copresence/mediums/audio/audio_manager_impl.cc:318: 1);
On 2015/02/04 23:37:18, Rahul Chaturvedi wrote:
> It isn't evident what this number means. Add a comment or make it a constant.

Done.

[ajm, 2015-02-05 00:03:41]

On 2015/02/04 23:41:02, Charlie wrote:
> On 2015/02/04 23:24:26, ajm wrote:
> > Just to be sure you're aware, there is some wav handling functionality in
> > Chromium proper:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/sounds...
> > 
> > which you might prefer to avoid a third_party dependency. Let me know what
you
> > think.
> 
> Isn't that just for input from a wav file? I'm trying to output to a wav file.

Ah, I missed that. Right, I don't see a facility for writing wav files in
Chromium.

Is this production code? As mentioned here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

CHECKs are used for error-checking. In webrtc we only use these classes for test
and debug code. It's possible this could be changed...

[Charlie, 2015-02-05 00:08:28]

On 2015/02/05 00:03:41, ajm wrote:
> On 2015/02/04 23:41:02, Charlie wrote:
> > On 2015/02/04 23:24:26, ajm wrote:
> > > Just to be sure you're aware, there is some wav handling functionality in
> > > Chromium proper:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/sounds...
> > > 
> > > which you might prefer to avoid a third_party dependency. Let me know what
> you
> > > think.
> > 
> > Isn't that just for input from a wav file? I'm trying to output to a wav
file.
> 
> Ah, I missed that. Right, I don't see a facility for writing wav files in
> Chromium.
> 
> Is this production code? As mentioned here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
> 
> CHECKs are used for error-checking. In webrtc we only use these classes for
test
> and debug code. It's possible this could be changed...

This is production code, but the flag is only for developers. If it's not set,
WavWriter is never invoked. I think it's ok for Chrome to abort if a
developer/tinkerer sets the flag and something goes wrong.

[rkc, 2015-02-05 00:14:03]

lgtm

[ajm, 2015-02-05 00:18:35]

DumpToken() lgtm % nits

https://codereview.chromium.org/892423002/diff/40001/components/copresence/me...
File components/copresence/mediums/audio/audio_manager_impl.cc (right):

https://codereview.chromium.org/892423002/diff/40001/components/copresence/me...
components/copresence/mediums/audio/audio_manager_impl.cc:313: DCHECK_EQ((int)
int_samples.size(), samples->frames());
static_cast?

Also, perhaps:
DCHECK_EQ(kMonoChannelCount, samples->channels())

https://codereview.chromium.org/892423002/diff/40001/components/copresence/me...
components/copresence/mediums/audio/audio_manager_impl.cc:316: " " +
ToUrlSafe(token) + ".wav";
Do you really want whitespace in a filename?

[Charlie, 2015-02-05 01:51:21]

New patchsets have been uploaded after l-g-t-m from
rkc@chromium.org,ajm@chromium.org

[Charlie, 2015-02-05 01:53:46]

Added a check for an invalid token directory, to reduce the likelihood of a
WavWriter crash. Feel free to take another look.

https://codereview.chromium.org/892423002/diff/40001/components/copresence/me...
File components/copresence/mediums/audio/audio_manager_impl.cc (right):

https://codereview.chromium.org/892423002/diff/40001/components/copresence/me...
components/copresence/mediums/audio/audio_manager_impl.cc:313: DCHECK_EQ((int)
int_samples.size(), samples->frames());
On 2015/02/05 00:18:35, ajm wrote:
> static_cast?
> 
> Also, perhaps:
> DCHECK_EQ(kMonoChannelCount, samples->channels())

Done.

https://codereview.chromium.org/892423002/diff/40001/components/copresence/me...
components/copresence/mediums/audio/audio_manager_impl.cc:316: " " +
ToUrlSafe(token) + ".wav";
On 2015/02/05 00:18:35, ajm wrote:
> Do you really want whitespace in a filename?

Yes. Unfortunately the tokens can contain underscores, so that's not a good
choice. And none of the other cross-platform-friendly delimiters were more
appealing.

[rkc, 2015-02-05 01:54:45]

https://codereview.chromium.org/892423002/diff/60001/components/copresence/me...
File components/copresence/mediums/audio/audio_manager_impl.h (right):

https://codereview.chromium.org/892423002/diff/60001/components/copresence/me...
components/copresence/mediums/audio/audio_manager_impl.h:84: void
ValidateTokenDir();
This is completely unnecessary. Once you are using special flags to Chrome, all
expectations of 'typical' behavior are gone. You are now in an *experimental*
mode, where if someone specifies a bad token dir, if Chrome crashes, that is
fine.

Adding code bloat for checking for a condition we care so little about, is
excessive.

[Charlie, 2015-02-05 02:01:56]

https://codereview.chromium.org/892423002/diff/60001/components/copresence/me...
File components/copresence/mediums/audio/audio_manager_impl.h (right):

https://codereview.chromium.org/892423002/diff/60001/components/copresence/me...
components/copresence/mediums/audio/audio_manager_impl.h:84: void
ValidateTokenDir();
On 2015/02/05 01:54:45, Rahul Chaturvedi wrote:
> This is completely unnecessary. Once you are using special flags to Chrome,
all
> expectations of 'typical' behavior are gone. You are now in an *experimental*
> mode, where if someone specifies a bad token dir, if Chrome crashes, that is
> fine.
> 
> Adding code bloat for checking for a condition we care so little about, is
> excessive.

Very well. Deleted.

[ajm, 2015-02-05 02:03:33]

lgtm

https://codereview.chromium.org/892423002/diff/40001/components/copresence/me...
File components/copresence/mediums/audio/audio_manager_impl.cc (right):

https://codereview.chromium.org/892423002/diff/40001/components/copresence/me...
components/copresence/mediums/audio/audio_manager_impl.cc:316: " " +
ToUrlSafe(token) + ".wav";
On 2015/02/05 01:53:46, Charlie wrote:
> On 2015/02/05 00:18:35, ajm wrote:
> > Do you really want whitespace in a filename?
> 
> Yes. Unfortunately the tokens can contain underscores, so that's not a good
> choice. And none of the other cross-platform-friendly delimiters were more
> appealing.

Acknowledged.

[rkc, 2015-02-05 02:26:26]

lgtm

[Charlie, 2015-02-05 02:33:48]

New patchsets have been uploaded after l-g-t-m from
ajm@chromium.org,rkc@chromium.org

[Charlie, 2015-02-05 02:34:59]

The CQ bit was checked by ckehoe@chromium.org

[commit-bot: I haz the power, 2015-02-05 02:36:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/892423002/100001

[commit-bot: I haz the power, 2015-02-05 02:41:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-05 02:41:50]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[chromium-reviews, 2015-02-05 02:51:21]

Andrew, can you LG from your webrtc account? Presubmit is failing:

** Presubmit ERRORS **
Missing LGTM from OWNERS of dependencies added to DEPS:
    '+third_party/webrtc/common_audio',



On Wed Feb 04 2015 at 6:41:51 PM <commit-bot@chromium.org> wrote:

> Try jobs failed on following builders:
>    chromium_presubmit on tryserver.chromium.linux
> (http://build.chromium.org/p/tryserver.chromium.linux/
> builders/chromium_presubmit/builds/40592)
>
> https://codereview.chromium.org/892423002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Charlie, 2015-02-05 04:02:30]

On 2015/02/05 01:54:45, Rahul Chaturvedi wrote:
>
https://codereview.chromium.org/892423002/diff/60001/components/copresence/me...
> File components/copresence/mediums/audio/audio_manager_impl.h (right):
> 
>
https://codereview.chromium.org/892423002/diff/60001/components/copresence/me...
> components/copresence/mediums/audio/audio_manager_impl.h:84: void
> ValidateTokenDir();
> This is completely unnecessary. Once you are using special flags to Chrome,
all
> expectations of 'typical' behavior are gone. You are now in an *experimental*
> mode, where if someone specifies a bad token dir, if Chrome crashes, that is
> fine.
> 
> Adding code bloat for checking for a condition we care so little about, is
> excessive.

Well, the code bloat is back, thanks to our friends Windows and wstring...

[Andrew MacDonald, 2015-02-05 05:56:38]

lgtm

[Charlie, 2015-02-05 06:11:32]

The CQ bit was checked by ckehoe@chromium.org

[commit-bot: I haz the power, 2015-02-05 06:12:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/892423002/140001

[commit-bot: I haz the power, 2015-02-05 07:35:53]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-02-05 07:36:44]

Patchset 8 (id:??) landed as
https://crrev.com/4f0acbe307357802844fbb6e974ce724dea45655
Cr-Commit-Position: refs/heads/master@{#314772}