Added option to specify a maximum file size when recording an AEC dump.

Added option to specify a maximum file size when recording an AEC dump.

For applications with a strict filesize limit for debug files,
I added an option to specify a maximum filesize for AEC dumps. An
existing unit test is extended to check that the feature works as
advertised.

BUG=webrtc:4741
TBR=glaznev@webrtc.org

Committed: https://crrev.com/ae2c5ad12afc8cc29fe9c59dea432b697b871a87
Cr-Commit-Position: refs/heads/master@{#11081}

[ivoc, 2015-10-23 12:58:10]

Description was changed from

==========
Added option to specify a maximum file size when recording an AEC dump.

For platforms with a strict filesize limit for uploading debug files,
there is now an option to specify a maximum filesize for AEC dumps. An
existing unit test is extended to check that the feature works as
advertised.

BUG=webrtc:4741
==========

to

==========
Added option to specify a maximum file size when recording an AEC dump.

For platforms with a strict filesize limit for uploading debug files,
I added an option to specify a maximum filesize for AEC dumps. An
existing unit test is extended to check that the feature works as
advertised.

BUG=webrtc:4741
==========

[ivoc, 2015-10-23 12:58:10]

ivoc@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org, pthatcher@webrtc.org

[ivoc, 2015-10-23 13:06:56]

ivoc@webrtc.org changed reviewers:
+ henrikg@webrtc.org

[ivoc, 2015-10-23 13:06:56]

ivoc@webrtc.org changed required reviewers:
+ henrik.lundin@webrtc.org, pthatcher@webrtc.org

[ivoc, 2015-10-23 13:10:32]

ivoc@webrtc.org changed required reviewers:
+ henrikg@webrtc.org

[ivoc, 2015-10-23 13:20:25]

Hi guys,

Please have a look at this CL to add a feature to cap the filesize of AEC dumps
to a specified upper bound. This is needed for applications with a strict
filesize limit for uploading debug files.

@pthatcher: feel free to focus on the files in talk/ if you're busy.

Thanks!

[Andrew MacDonald, 2015-10-23 17:15:17]

andrew@webrtc.org changed reviewers:
+ andrew@webrtc.org, solenberg@webrtc.org

[Andrew MacDonald, 2015-10-23 17:15:18]

Drive-by.

+the sun, question about refactoring

I didn't look at the implementation: does this stop dumping after the cap has
been reached, or restart the dump? Not immediately sure which is more useful,
but the interface should be clear on the matter.

https://codereview.webrtc.org/1413483003/diff/1/webrtc/voice_engine/include/v...
File webrtc/voice_engine/include/voe_audio_processing.h (right):

https://codereview.webrtc.org/1413483003/diff/1/webrtc/voice_engine/include/v...
webrtc/voice_engine/include/voe_audio_processing.h:194: virtual int
StartDebugRecording(const char* fileNameUTF8,
The preferred method now is to access the AudioProcessing instance directly in
libjingle, through:
voe_wrapper_->base()->audio_processing()

This wrapper layer should go away entirely with solenberg's refactoring; it
would be nicer to avoid adding new APIs, but I leave it up to you guys.

[kwiberg-webrtc, 2015-10-25 02:29:11]

kwiberg@webrtc.org changed reviewers:
+ kwiberg@webrtc.org

[kwiberg-webrtc, 2015-10-25 02:29:12]

https://codereview.webrtc.org/1413483003/diff/1/talk/media/webrtc/webrtcvoice...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1413483003/diff/1/talk/media/webrtc/webrtcvoice...
talk/media/webrtc/webrtcvoiceengine.cc:1300: }
This duplicates a lot of the preceding function. Can you make one of them call
the other, or extract a new method for both of them to call?

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/audio_processing_impl.cc (right):

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:1229:
nr_bytes_left_for_log -= (sizeof(int32_t) + event_str_.size());
You use event_str_.length() below. Are they the same? You should probably
standardize on one of them even if they are.

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/audio_processing_impl.h (right):

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.h:183: int
nr_bytes_left_for_log;
Split comment into separate sentences at the comma. And underscore at the end of
the name.

Oh, and using int means that you can't give a limit larger than 2 GB - 1 byte.
Probably not an immediate concern, but probably worth fixing now anyway while
it's easy.

https://codereview.webrtc.org/1413483003/diff/1/webrtc/voice_engine/voe_audio...
File webrtc/voice_engine/voe_audio_processing_impl.cc (right):

https://codereview.webrtc.org/1413483003/diff/1/webrtc/voice_engine/voe_audio...
webrtc/voice_engine/voe_audio_processing_impl.cc:944: }
Duplicates code from the preceding function. Can you make one of them call the
other?

https://codereview.webrtc.org/1413483003/diff/1/webrtc/voice_engine/voe_audio...
webrtc/voice_engine/voe_audio_processing_impl.cc:968: }
Code dup; one call the other?

[the sun, 2015-10-26 10:37:20]

https://codereview.webrtc.org/1413483003/diff/1/talk/app/webrtc/peerconnectio...
File talk/app/webrtc/peerconnectionfactory.h (right):

https://codereview.webrtc.org/1413483003/diff/1/talk/app/webrtc/peerconnectio...
talk/app/webrtc/peerconnectionfactory.h:83: bool StartAecDump(rtc::PlatformFile
file, int max_size_bytes) override;
Is the plan to subsequently remove the previous function? For this type of
interface I think it makes sense to allow max_size_bytes <= 0 to mean "no
limit".

To support the old interface until the clients are updated, you could do:

bool StartAecDump(rtc::PlatformFile file) override {
  StartAecDump(file, 0);
}

And then we could remove some of the duplicated code further down the chain.

https://codereview.webrtc.org/1413483003/diff/1/talk/media/webrtc/webrtcvoice...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1413483003/diff/1/talk/media/webrtc/webrtcvoice...
talk/media/webrtc/webrtcvoiceengine.cc:1300: }
On 2015/10/25 02:29:12, kwiberg-webrtc wrote:
> This duplicates a lot of the preceding function. Can you make one of them call
> the other, or extract a new method for both of them to call?

See previous comment in PeerConnectionFactory: we can make do with just this
method here.

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/audio_processing_impl.cc (right):

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:1233:
debug_file_->CloseFile();
Can you add:
RTC_DCHECK(debug_file_->Open());
at the start of this function?

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/audio_processing_impl.h (right):

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.h:106: int
StartDebugRecording(const char filename[kMaxFilenameSize],
I think it makes sense to keep just the interface with a given size here as
well.

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/include/audio_processing.h (right):

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/include/audio_processing.h:398: virtual int
StartDebugRecording(const char filename[kMaxFilenameSize],
Consolidate to one interface, but keep a forwarding impl of the old version
until Chromium code has been updated.

https://codereview.webrtc.org/1413483003/diff/1/webrtc/voice_engine/include/v...
File webrtc/voice_engine/include/voe_audio_processing.h (right):

https://codereview.webrtc.org/1413483003/diff/1/webrtc/voice_engine/include/v...
webrtc/voice_engine/include/voe_audio_processing.h:194: virtual int
StartDebugRecording(const char* fileNameUTF8,
On 2015/10/23 17:15:18, Andrew MacDonald wrote:
> The preferred method now is to access the AudioProcessing instance directly in
> libjingle, through:
> voe_wrapper_->base()->audio_processing()
> 
> This wrapper layer should go away entirely with solenberg's refactoring; it
> would be nicer to avoid adding new APIs, but I leave it up to you guys.

Thanks Andrew; indeed we don't want to and don't have to add the extra
interfaces here since we can get to the APM directly.

[hlundin-webrtc, 2015-10-30 08:02:21]

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/audio_processing_impl.cc (right):

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:941:
nr_bytes_left_for_log = -1;
Is it important to reset to -1 when an error happens?

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:1233:
debug_file_->CloseFile();
On 2015/10/26 10:37:20, the sun wrote:
> Can you add:
> RTC_DCHECK(debug_file_->Open());
> at the start of this function?

I agree. I'm also a bit uneasy about setting nr_bytes_left_for_log = -1 after
having written the allowed file size. To the casual reader, this looks like
removing the limit once the limit was removed.

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/audio_processing_impl.h (right):

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.h:183: int
nr_bytes_left_for_log;
On 2015/10/25 02:29:12, kwiberg-webrtc wrote:
> Split comment into separate sentences at the comma. And underscore at the end
of
> the name.
> 
> Oh, and using int means that you can't give a limit larger than 2 GB - 1 byte.
> Probably not an immediate concern, but probably worth fixing now anyway while
> it's easy.

Yes, and in general, variables dealing with number of bytes, indices, and counts
should be size_t. But I do realize you get a problem with the semantics for not
setting a limit. Let's discuss offline.

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/test/audio_processing_unittest.cc (right):

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/audio_processing_unittest.cc:1858:
EXPECT_LE(bytes_read_limited, logging_limit_bytes);
Should you maybe test that the actual file size ends up being somewhat close to
the target/limit?

[kwiberg-webrtc, 2015-10-30 09:53:21]

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/audio_processing_impl.h (right):

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.h:183: int
nr_bytes_left_for_log;
On 2015/10/30 08:02:21, hlundin-webrtc wrote:
> On 2015/10/25 02:29:12, kwiberg-webrtc wrote:
> > Split comment into separate sentences at the comma. And underscore at the
end
> of
> > the name.
> > 
> > Oh, and using int means that you can't give a limit larger than 2 GB - 1
byte.
> > Probably not an immediate concern, but probably worth fixing now anyway
while
> > it's easy.
> 
> Yes, and in general, variables dealing with number of bytes, indices, and
counts
> should be size_t. But I do realize you get a problem with the semantics for
not
> setting a limit. Let's discuss offline.

Variables dealing with number of bytes, indices, and counts *in RAM* should be
size_t. For counting the same things *on disk*, use int64_t or uint64_t
(opinions vary on which is the right one; either one will work just fine).
Otherwise, 32-bit systems won't be able to handle files larger than 4 GB.

[hlundin-webrtc, 2015-10-30 09:59:15]

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/audio_processing_impl.h (right):

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.h:183: int
nr_bytes_left_for_log;
On 2015/10/30 09:53:21, kwiberg-webrtc wrote:
> On 2015/10/30 08:02:21, hlundin-webrtc wrote:
> > On 2015/10/25 02:29:12, kwiberg-webrtc wrote:
> > > Split comment into separate sentences at the comma. And underscore at the
> end
> > of
> > > the name.
> > > 
> > > Oh, and using int means that you can't give a limit larger than 2 GB - 1
> byte.
> > > Probably not an immediate concern, but probably worth fixing now anyway
> while
> > > it's easy.
> > 
> > Yes, and in general, variables dealing with number of bytes, indices, and
> counts
> > should be size_t. But I do realize you get a problem with the semantics for
> not
> > setting a limit. Let's discuss offline.
> 
> Variables dealing with number of bytes, indices, and counts *in RAM* should be
> size_t. For counting the same things *on disk*, use int64_t or uint64_t
> (opinions vary on which is the right one; either one will work just fine).
> Otherwise, 32-bit systems won't be able to handle files larger than 4 GB.

Ack. I stand corrected.

[ivoc, 2015-11-05 13:09:34]

Patchset #2 (id:20001) has been deleted

[ivoc, 2015-11-05 13:14:46]

Thanks for the great comments! Sorry for the delay, something else popped up. 

Where possible I removed the old function without a limit in bytes like many of
you suggested, and elsewhere I reimplemented the old function using the new
function. The size in bytes is changed from int to int64_t throughout all of the
calls, and a value <= 0 means no limit should be used.

https://codereview.webrtc.org/1413483003/diff/1/talk/app/webrtc/peerconnectio...
File talk/app/webrtc/peerconnectionfactory.h (right):

https://codereview.webrtc.org/1413483003/diff/1/talk/app/webrtc/peerconnectio...
talk/app/webrtc/peerconnectionfactory.h:83: bool StartAecDump(rtc::PlatformFile
file, int max_size_bytes) override;
On 2015/10/26 10:37:20, the sun wrote:
> Is the plan to subsequently remove the previous function? For this type of
> interface I think it makes sense to allow max_size_bytes <= 0 to mean "no
> limit".
> 
> To support the old interface until the clients are updated, you could do:
> 
> bool StartAecDump(rtc::PlatformFile file) override {
>   StartAecDump(file, 0);
> }
> 
> And then we could remove some of the duplicated code further down the chain.

Since the previous function is not used (as far as I'm aware), it makes sense to
just remove it completely, which I did.

https://codereview.webrtc.org/1413483003/diff/1/talk/media/webrtc/webrtcvoice...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1413483003/diff/1/talk/media/webrtc/webrtcvoice...
talk/media/webrtc/webrtcvoiceengine.cc:1300: }
On 2015/10/26 10:37:20, the sun wrote:
> On 2015/10/25 02:29:12, kwiberg-webrtc wrote:
> > This duplicates a lot of the preceding function. Can you make one of them
call
> > the other, or extract a new method for both of them to call?
> 
> See previous comment in PeerConnectionFactory: we can make do with just this
> method here.

Done.

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/audio_processing_impl.cc (right):

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:941:
nr_bytes_left_for_log = -1;
On 2015/10/30 08:02:21, hlundin-webrtc wrote:
> Is it important to reset to -1 when an error happens?

I did this because there was also the other version of this function that does
not have a max size, however, I reimplemented that one in terms of this
function, so this is no longer needed.

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:1229:
nr_bytes_left_for_log -= (sizeof(int32_t) + event_str_.size());
On 2015/10/25 02:29:12, kwiberg-webrtc wrote:
> You use event_str_.length() below. Are they the same? You should probably
> standardize on one of them even if they are.

for std::string, .size() and .length() are exactly the same. I personally like
.size() because it's the same across all of the STL containers, but since the
old code below already used .length() I will change this code to also use it.

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:1233:
debug_file_->CloseFile();
On 2015/10/30 08:02:21, hlundin-webrtc wrote:
> On 2015/10/26 10:37:20, the sun wrote:
> > Can you add:
> > RTC_DCHECK(debug_file_->Open());
> > at the start of this function?
> 
> I agree. I'm also a bit uneasy about setting nr_bytes_left_for_log = -1 after
> having written the allowed file size. To the casual reader, this looks like
> removing the limit once the limit was removed.

I added the RTC_DCHECK. I removed the code setting the nr_bytes_left_for_log to
-1, since it is no longer necessary now.

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/audio_processing_impl.h (right):

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.h:106: int
StartDebugRecording(const char filename[kMaxFilenameSize],
On 2015/10/26 10:37:20, the sun wrote:
> I think it makes sense to keep just the interface with a given size here as
> well.

Done.

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.h:183: int
nr_bytes_left_for_log;
On 2015/10/30 09:59:14, hlundin-webrtc wrote:
> On 2015/10/30 09:53:21, kwiberg-webrtc wrote:
> > On 2015/10/30 08:02:21, hlundin-webrtc wrote:
> > > On 2015/10/25 02:29:12, kwiberg-webrtc wrote:
> > > > Split comment into separate sentences at the comma. And underscore at
the
> > end
> > > of
> > > > the name.
> > > > 
> > > > Oh, and using int means that you can't give a limit larger than 2 GB - 1
> > byte.
> > > > Probably not an immediate concern, but probably worth fixing now anyway
> > while
> > > > it's easy.
> > > 
> > > Yes, and in general, variables dealing with number of bytes, indices, and
> > counts
> > > should be size_t. But I do realize you get a problem with the semantics
for
> > not
> > > setting a limit. Let's discuss offline.
> > 
> > Variables dealing with number of bytes, indices, and counts *in RAM* should
be
> > size_t. For counting the same things *on disk*, use int64_t or uint64_t
> > (opinions vary on which is the right one; either one will work just fine).
> > Otherwise, 32-bit systems won't be able to handle files larger than 4 GB.
> 
> Ack. I stand corrected.

I went for int64_t, hope that's okay.

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/include/audio_processing.h (right):

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/include/audio_processing.h:398: virtual int
StartDebugRecording(const char filename[kMaxFilenameSize],
On 2015/10/26 10:37:20, the sun wrote:
> Consolidate to one interface, but keep a forwarding impl of the old version
> until Chromium code has been updated.

Done.

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/test/audio_processing_unittest.cc (right):

https://codereview.webrtc.org/1413483003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/audio_processing_unittest.cc:1858:
EXPECT_LE(bytes_read_limited, logging_limit_bytes);
On 2015/10/30 08:02:21, hlundin-webrtc wrote:
> Should you maybe test that the actual file size ends up being somewhat close
to
> the target/limit?

I added an appropriate lower bound.

https://codereview.webrtc.org/1413483003/diff/1/webrtc/voice_engine/include/v...
File webrtc/voice_engine/include/voe_audio_processing.h (right):

https://codereview.webrtc.org/1413483003/diff/1/webrtc/voice_engine/include/v...
webrtc/voice_engine/include/voe_audio_processing.h:194: virtual int
StartDebugRecording(const char* fileNameUTF8,
On 2015/10/26 10:37:20, the sun wrote:
> On 2015/10/23 17:15:18, Andrew MacDonald wrote:
> > The preferred method now is to access the AudioProcessing instance directly
in
> > libjingle, through:
> > voe_wrapper_->base()->audio_processing()
> > 
> > This wrapper layer should go away entirely with solenberg's refactoring; it
> > would be nicer to avoid adding new APIs, but I leave it up to you guys.
> 
> Thanks Andrew; indeed we don't want to and don't have to add the extra
> interfaces here since we can get to the APM directly.

I removed this function, should I remove the StartDebugRecording() function that
was already here as well?

https://codereview.webrtc.org/1413483003/diff/1/webrtc/voice_engine/voe_audio...
File webrtc/voice_engine/voe_audio_processing_impl.cc (right):

https://codereview.webrtc.org/1413483003/diff/1/webrtc/voice_engine/voe_audio...
webrtc/voice_engine/voe_audio_processing_impl.cc:944: }
On 2015/10/25 02:29:12, kwiberg-webrtc wrote:
> Duplicates code from the preceding function. Can you make one of them call the
> other?

This code has been removed in the new version.

https://codereview.webrtc.org/1413483003/diff/1/webrtc/voice_engine/voe_audio...
webrtc/voice_engine/voe_audio_processing_impl.cc:968: }
On 2015/10/25 02:29:12, kwiberg-webrtc wrote:
> Code dup; one call the other?

This code has been removed in the new version.

[ivoc, 2015-11-06 08:34:31]

Description was changed from

==========
Added option to specify a maximum file size when recording an AEC dump.

For platforms with a strict filesize limit for uploading debug files,
I added an option to specify a maximum filesize for AEC dumps. An
existing unit test is extended to check that the feature works as
advertised.

BUG=webrtc:4741
==========

to

==========
Added option to specify a maximum file size when recording an AEC dump.

For applications with a strict filesize limit for debug files,
I added an option to specify a maximum filesize for AEC dumps. An
existing unit test is extended to check that the feature works as
advertised.

BUG=webrtc:4741
==========

[kwiberg-webrtc, 2015-11-08 19:09:07]

lgtm

https://codereview.webrtc.org/1413483003/diff/40001/talk/media/base/mediaengi...
File talk/media/base/mediaengine.h (right):

https://codereview.webrtc.org/1413483003/diff/40001/talk/media/base/mediaengi...
talk/media/base/mediaengine.h:124: // stopped and the file is closed. If
max_size_bytes is set to a value <= 0,
Is it really the case that logging stops *after* the file has exceeded the
limit?

[the sun, 2015-11-11 15:54:42]

https://codereview.webrtc.org/1413483003/diff/40001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1413483003/diff/40001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:1263: int64_t max_size_bytes) {
I don't see where max_size_bytes is used in this function

https://codereview.webrtc.org/1413483003/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/audio_processing_impl.cc (right):

https://codereview.webrtc.org/1413483003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/audio_processing_impl.cc:565:
RETURN_ON_ERR(WriteMessageToDebugFile());
What happens when ProcessStream() suddenly returns an error (because
WriteMessageToDebugFile() overshot the limit)?

Maybe it is better to not return an error from WriteMessageToDebugFile()?

https://codereview.webrtc.org/1413483003/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/include/audio_processing.h (right):

https://codereview.webrtc.org/1413483003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/include/audio_processing.h:405: int
StartDebugRecording(FILE* handle) {
Are both of these functions in use, or can we drop one of them?

[hlundin-webrtc, 2015-11-11 15:55:05]

webrtc/*: lgtm with one nit.

https://codereview.webrtc.org/1413483003/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/audio_processing_impl.h (right):

https://codereview.webrtc.org/1413483003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/audio_processing_impl.h:181: int64_t
nr_bytes_left_for_log;
Trailing underscore on member variable names.

[ivoc, 2015-11-11 16:44:32]

Thanks for the reviews, see my replies below.

https://codereview.webrtc.org/1413483003/diff/40001/talk/media/base/mediaengi...
File talk/media/base/mediaengine.h (right):

https://codereview.webrtc.org/1413483003/diff/40001/talk/media/base/mediaengi...
talk/media/base/mediaengine.h:124: // stopped and the file is closed. If
max_size_bytes is set to a value <= 0,
On 2015/11/08 19:09:07, kwiberg-webrtc wrote:
> Is it really the case that logging stops *after* the file has exceeded the
> limit?

It is not, fixed the comment.

https://codereview.webrtc.org/1413483003/diff/40001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1413483003/diff/40001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:1263: int64_t max_size_bytes) {
On 2015/11/11 15:54:42, the sun wrote:
> I don't see where max_size_bytes is used in this function

Wow, good find, what was I thinking? :-)

https://codereview.webrtc.org/1413483003/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/audio_processing_impl.cc (right):

https://codereview.webrtc.org/1413483003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/audio_processing_impl.cc:565:
RETURN_ON_ERR(WriteMessageToDebugFile());
On 2015/11/11 15:54:42, the sun wrote:
> What happens when ProcessStream() suddenly returns an error (because
> WriteMessageToDebugFile() overshot the limit)?
> 
> Maybe it is better to not return an error from WriteMessageToDebugFile()?

Good point, I changed the return code when the size limit is reached to
kNoError.

https://codereview.webrtc.org/1413483003/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/audio_processing_impl.h (right):

https://codereview.webrtc.org/1413483003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/audio_processing_impl.h:181: int64_t
nr_bytes_left_for_log;
On 2015/11/11 15:55:05, hlundin-webrtc wrote:
> Trailing underscore on member variable names.

Right, fixed.

https://codereview.webrtc.org/1413483003/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/include/audio_processing.h (right):

https://codereview.webrtc.org/1413483003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/include/audio_processing.h:405: int
StartDebugRecording(FILE* handle) {
On 2015/11/11 15:54:42, the sun wrote:
> Are both of these functions in use, or can we drop one of them?

It is used internally in a few places, mainly in the unit tests. I went through
each call and replaced them with the other version, and then I removed this
function and the char[] based one as well.

[ivoc, 2015-11-24 09:33:53]

Hi guys, we need this CL to go in before we can add AEC dumps to mobile
applications, so it would be great if you can have a look at this any time soon.
Thanks!

[the sun, 2015-11-24 10:10:10]

lgtm

https://codereview.webrtc.org/1413483003/diff/60001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/audio_processing_impl.h (right):

https://codereview.webrtc.org/1413483003/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/audio_processing_impl.h:181: int64_t
nr_bytes_left_for_log_;
nit: nr_ -> num_

[Andrew MacDonald, 2015-11-24 17:28:59]

https://codereview.webrtc.org/1413483003/diff/60001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/audio_processing_impl.cc (left):

https://codereview.webrtc.org/1413483003/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/audio_processing_impl.cc:914: if
(debug_file_->Open()) {
Why did you remove this block? It's not removed from the overloaded method
below.

https://codereview.webrtc.org/1413483003/diff/60001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/audio_processing_impl.h (right):

https://codereview.webrtc.org/1413483003/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/audio_processing_impl.h:181: int64_t
nr_bytes_left_for_log_;
On 2015/11/24 10:10:10, the sun wrote:
> nit: nr_ -> num_

+1

https://codereview.webrtc.org/1413483003/diff/60001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/include/audio_processing.h (right):

https://codereview.webrtc.org/1413483003/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/include/audio_processing.h:403: // limit has
been reached.
nit: Just refer to the comments above rather than repeating.

https://codereview.webrtc.org/1413483003/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/include/audio_processing.h:409: virtual int
StartDebugRecordingForPlatformFile(rtc::PlatformFile handle) {
Any reason this one doesn't take a max size parameter?

[Henrik Grunell WebRTC, 2015-11-26 12:42:55]

High level lgtm.

[ivoc, 2015-12-01 15:06:29]

Patchset #4 (id:80001) has been deleted

[ivoc, 2015-12-01 15:17:17]

https://codereview.webrtc.org/1413483003/diff/60001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/audio_processing_impl.cc (left):

https://codereview.webrtc.org/1413483003/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/audio_processing_impl.cc:914: if
(debug_file_->Open()) {
On 2015/11/24 17:28:59, Andrew MacDonald wrote:
> Why did you remove this block? It's not removed from the overloaded method
> below.

Good point, I'm not sure why I removed it initially, so I will put it back.

https://codereview.webrtc.org/1413483003/diff/60001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/audio_processing_impl.h (right):

https://codereview.webrtc.org/1413483003/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/audio_processing_impl.h:181: int64_t
nr_bytes_left_for_log_;
On 2015/11/24 17:28:59, Andrew MacDonald wrote:
> On 2015/11/24 10:10:10, the sun wrote:
> > nit: nr_ -> num_
> 
> +1

Done.

https://codereview.webrtc.org/1413483003/diff/60001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/include/audio_processing.h (right):

https://codereview.webrtc.org/1413483003/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/include/audio_processing.h:403: // limit has
been reached.
On 2015/11/24 17:28:59, Andrew MacDonald wrote:
> nit: Just refer to the comments above rather than repeating.

Done.

https://codereview.webrtc.org/1413483003/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/include/audio_processing.h:409: virtual int
StartDebugRecordingForPlatformFile(rtc::PlatformFile handle) {
I think this is the function that Chrome calls to start recording AEC dumps. I
didn't want to touch it to prevent Chrome breakage.

[Andrew MacDonald, 2015-12-02 00:11:13]

lgtm

https://codereview.webrtc.org/1413483003/diff/60001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/include/audio_processing.h (right):

https://codereview.webrtc.org/1413483003/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/include/audio_processing.h:409: virtual int
StartDebugRecordingForPlatformFile(rtc::PlatformFile handle) {
On 2015/12/01 15:17:16, ivoc wrote:
> I think this is the function that Chrome calls to start recording AEC dumps. I
> didn't want to touch it to prevent Chrome breakage.

OK. Perhaps mention that no logging limit will be used for this interface.

[ivoc, 2015-12-07 08:46:32]

ivoc@webrtc.org changed reviewers:
+ perkj@webrtc.org

[ivoc, 2015-12-07 08:46:32]

ivoc@webrtc.org changed required reviewers:
- pthatcher@webrtc.org

[ivoc, 2015-12-07 08:49:40]

@perkj: Could you have a look at the changes in talk/ please? Thanks!

[perkj_webrtc, 2015-12-09 20:07:49]

I think this will break chrome unless you also keep the previous  version in
peerconnectioninterface until you have rolled this in and fixed chrome.

You can add back the old method in peerconnectioninterface.h and call the new
one with max_size=0.  and add a todo to remove it once it has been updated in
Chrome.

lgtm

[Henrik Grunell WebRTC, 2015-12-10 09:28:38]

On 2015/12/09 20:07:49, perkj1 wrote:
> I think this will break chrome unless you also keep the previous  version in
> peerconnectioninterface until you have rolled this in and fixed chrome.
> 
> You can add back the old method in peerconnectioninterface.h and call the new
> one with max_size=0.  and add a todo to remove it once it has been updated in
> Chrome.
> 
> lgtm

Good point. (I hadn't looked at the talk/ files, maybe I should have, but I
suspect Peter would have caught this.)

[ivoc, 2015-12-10 10:06:10]

On 2015/12/10 09:28:38, Henrik Grunell (webrtc) wrote:
> On 2015/12/09 20:07:49, perkj1 wrote:
> > I think this will break chrome unless you also keep the previous  version in
> > peerconnectioninterface until you have rolled this in and fixed chrome.
> > 
> > You can add back the old method in peerconnectioninterface.h and call the
new
> > one with max_size=0.  and add a todo to remove it once it has been updated
in
> > Chrome.
> > 
> > lgtm
> 
> Good point. (I hadn't looked at the talk/ files, maybe I should have, but I
> suspect Peter would have caught this.)

As far as I know, chrome does not call the AEC dump start/stop functions in
peerconnectioninterface.h, but instead calls the start/stop functions on
audio_processing.h directly. Until recently there was not even a stop function
available in peerconnectioninterface.h (I added it recently).

[Henrik Grunell WebRTC, 2015-12-10 17:41:30]

On 2015/12/10 10:06:10, ivoc wrote:
> On 2015/12/10 09:28:38, Henrik Grunell (webrtc) wrote:
> > On 2015/12/09 20:07:49, perkj1 wrote:
> > > I think this will break chrome unless you also keep the previous  version
in
> > > peerconnectioninterface until you have rolled this in and fixed chrome.
> > > 
> > > You can add back the old method in peerconnectioninterface.h and call the
> new
> > > one with max_size=0.  and add a todo to remove it once it has been updated
> in
> > > Chrome.
> > > 
> > > lgtm
> > 
> > Good point. (I hadn't looked at the talk/ files, maybe I should have, but I
> > suspect Peter would have caught this.)
> 
> As far as I know, chrome does not call the AEC dump start/stop functions in
> peerconnectioninterface.h, but instead calls the start/stop functions on
> audio_processing.h directly. Until recently there was not even a stop function
> available in peerconnectioninterface.h (I added it recently).

You're right, that path has been completely removed in Chromium. I verified that
just now.

[ivoc, 2015-12-15 09:28:41]

ivoc@webrtc.org changed reviewers:
+ glaznev@webrtc.org

[ivoc, 2015-12-15 09:33:18]

I had to make some more changes due to a rebase. glaznev@: Could you have a look
at the changes in talk/app/webrtc/java/ and webrtc/examples/ ? Thanks!

[ivoc, 2015-12-18 09:23:56]

Description was changed from

==========
Added option to specify a maximum file size when recording an AEC dump.

For applications with a strict filesize limit for debug files,
I added an option to specify a maximum filesize for AEC dumps. An
existing unit test is extended to check that the feature works as
advertised.

BUG=webrtc:4741
==========

to

==========
Added option to specify a maximum file size when recording an AEC dump.

For applications with a strict filesize limit for debug files,
I added an option to specify a maximum filesize for AEC dumps. An
existing unit test is extended to check that the feature works as
advertised.

BUG=webrtc:4741
TBR=glaznev@webrtc.org
==========

[ivoc, 2015-12-18 09:27:54]

I'm going to TBR this because it's my last day before my (rather long) vacation.

[ivoc, 2015-12-18 09:28:08]

The CQ bit was checked by ivoc@webrtc.org

[ivoc, 2015-12-18 09:28:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org, kwiberg@webrtc.org, solenberg@webrtc.org,
henrikg@webrtc.org, andrew@webrtc.org, perkj@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1413483003/#ps120001 (title:
"Rebase and made small update to JNI layer.")

[commit-bot: I haz the power, 2015-12-18 09:28:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1413483003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1413483003/120001

[commit-bot: I haz the power, 2015-12-18 11:53:41]

Description was changed from

==========
Added option to specify a maximum file size when recording an AEC dump.

For applications with a strict filesize limit for debug files,
I added an option to specify a maximum filesize for AEC dumps. An
existing unit test is extended to check that the feature works as
advertised.

BUG=webrtc:4741
TBR=glaznev@webrtc.org
==========

to

==========
Added option to specify a maximum file size when recording an AEC dump.

For applications with a strict filesize limit for debug files,
I added an option to specify a maximum filesize for AEC dumps. An
existing unit test is extended to check that the feature works as
advertised.

BUG=webrtc:4741
TBR=glaznev@webrtc.org
==========

[commit-bot: I haz the power, 2015-12-18 11:53:42]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2015-12-18 11:53:47]

Description was changed from

==========
Added option to specify a maximum file size when recording an AEC dump.

For applications with a strict filesize limit for debug files,
I added an option to specify a maximum filesize for AEC dumps. An
existing unit test is extended to check that the feature works as
advertised.

BUG=webrtc:4741
TBR=glaznev@webrtc.org
==========

to

==========
Added option to specify a maximum file size when recording an AEC dump.

For applications with a strict filesize limit for debug files,
I added an option to specify a maximum filesize for AEC dumps. An
existing unit test is extended to check that the feature works as
advertised.

BUG=webrtc:4741
TBR=glaznev@webrtc.org

Committed: https://crrev.com/ae2c5ad12afc8cc29fe9c59dea432b697b871a87
Cr-Commit-Position: refs/heads/master@{#11081}
==========

[commit-bot: I haz the power, 2015-12-18 11:53:49]

Patchset 5 (id:??) landed as
https://crrev.com/ae2c5ad12afc8cc29fe9c59dea432b697b871a87
Cr-Commit-Position: refs/heads/master@{#11081}

[ivoc, 2015-12-18 16:04:45]

A revert of this CL (patchset #5 id:120001) has been created in
https://codereview.webrtc.org/1533913004/ by ivoc@webrtc.org.

The reason for reverting is: Breaks Chrome-FYI bots because of a change in the
StartDebugRecording function in audio_processing.h, that is called from Chrome.
.