NetEq: Limit payload size for replacement audio input

NetEq: Limit payload size for replacement audio input

With this fix, the size of the fake encoded payload is limited to 120
ms at 48000 samples/second.

BUG=webrtc:7467
Review-Url: https://codereview.webrtc.org/2838353002
Cr-Commit-Position: refs/heads/master@{#17891}
Committed: https://chromium.googlesource.com/external/webrtc/+/65881de6c81cf3f33b0f2f5f8f4a19716ee93fdc

[hlundin-webrtc, 2017-04-26 11:37:51]

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

[hlundin-webrtc, 2017-04-26 11:37:51]

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

[hlundin-webrtc, 2017-04-26 11:37:51]

Please, take look.

[ivoc, 2017-04-26 12:03:22]

lgtm.

[hlundin-webrtc, 2017-04-26 12:12:19]

The CQ bit was checked by henrik.lundin@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 12:12:23]

Dry run: 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-26 12:29:13]

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

[commit-bot: I haz the power, 2017-04-26 12:29:14]

Dry run: This issue passed the CQ dry run.

[AleBzk, 2017-04-26 13:26:48]

LGTM with a couple of comments from a newbie

https://codereview.webrtc.org/2838353002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/tools/neteq_replacement_input.cc (right):

https://codereview.webrtc.org/2838353002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/tools/neteq_replacement_input.cc:93: //
Packets are in order.
If relevant, say something about the additional condition in the if statement.

https://codereview.webrtc.org/2838353002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/tools/neteq_replacement_input.cc:95:
last_frame_size_timestamps_ = input_frame_size_timestamps;
Maybe, you also want to initialize last_frame_size_timestamps_ to 120 * 48. I
checked in the .h file
(https://chromium.googlesource.com/external/webrtc/+/master/webrtc/modules/aud...),
it's initialized to a smaller value; so the limiting behavior is correct. 

It's safer to DCHECK that last_frame_size_timestamps_ is initialized with a
value not greater than 120 * 48. WDYT?

[hlundin-webrtc, 2017-04-26 13:58:51]

Thanks. Please, see new patch set.

https://codereview.webrtc.org/2838353002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/tools/neteq_replacement_input.cc (right):

https://codereview.webrtc.org/2838353002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/tools/neteq_replacement_input.cc:93: //
Packets are in order.
On 2017/04/26 13:26:48, AleBzk wrote:
> If relevant, say something about the additional condition in the if statement.

Done.

https://codereview.webrtc.org/2838353002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/tools/neteq_replacement_input.cc:95:
last_frame_size_timestamps_ = input_frame_size_timestamps;
On 2017/04/26 13:26:48, AleBzk wrote:
> Maybe, you also want to initialize last_frame_size_timestamps_ to 120 * 48. I
> checked in the .h file
>
(https://chromium.googlesource.com/external/webrtc/+/master/webrtc/modules/aud...),
> it's initialized to a smaller value; so the limiting behavior is correct. 
> 
> It's safer to DCHECK that last_frame_size_timestamps_ is initialized with a
> value not greater than 120 * 48. WDYT?

I added a DCHECK. Does this match you thinking?

[AleBzk, 2017-04-26 14:26:48]

Thanks for the change, I replied below:

https://codereview.webrtc.org/2838353002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tools/neteq_replacement_input.cc (right):

https://codereview.webrtc.org/2838353002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_replacement_input.cc:98:
RTC_DCHECK_LE(input_frame_size_timestamps, 120 * 48);
I'd rather do this before line 88 and on last_frame_size_timestamps_, not
input_frame_size_timestamps.

Let me make an example.
Imagine that last_frame_size_timestamps_ is initialized to 300 * 48 (or any
other value > 120 * 48).
Then, until you don't come across a timestamp_diff <= 120 * 48,
last_frame_size_timestamps_ is not updated and
FakeDecodeFromFile::PrepareEncoded() is called using 300 * 48.

If your intention is to always limit to 120ms, then you need to move and change
the DCHECK as I proposed above. In this way, if someone changes the init value
for last_frame_size_timestamps_ in the .h file, and the value is > 120 * 48, the
code fails.

[hlundin-webrtc, 2017-04-26 14:32:32]

Will land this patch set.

https://codereview.webrtc.org/2838353002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tools/neteq_replacement_input.cc (right):

https://codereview.webrtc.org/2838353002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_replacement_input.cc:98:
RTC_DCHECK_LE(input_frame_size_timestamps, 120 * 48);
On 2017/04/26 14:26:48, AleBzk wrote:
> I'd rather do this before line 88 and on last_frame_size_timestamps_, not
> input_frame_size_timestamps.
> 
> Let me make an example.
> Imagine that last_frame_size_timestamps_ is initialized to 300 * 48 (or any
> other value > 120 * 48).
> Then, until you don't come across a timestamp_diff <= 120 * 48,
> last_frame_size_timestamps_ is not updated and
> FakeDecodeFromFile::PrepareEncoded() is called using 300 * 48.
> 
> If your intention is to always limit to 120ms, then you need to move and
change
> the DCHECK as I proposed above. In this way, if someone changes the init value
> for last_frame_size_timestamps_ in the .h file, and the value is > 120 * 48,
the
> code fails.

I see your point. But I think that we are not as much documenting what the init
should be as asserting that at the end of the day PrepareEncoded will be called
with a sane value.

I did both now. :)

[hlundin-webrtc, 2017-04-26 14:32:45]

The CQ bit was checked by henrik.lundin@webrtc.org

[hlundin-webrtc, 2017-04-26 14:32:47]

The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org,
alessiob@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2838353002/#ps40001 (title:
"One more DCHECK")

[commit-bot: I haz the power, 2017-04-26 14:32:57]

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-26 15:23:35]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493217165589050,
"parent_rev": "afcf7f5591de47063f7da3507ea83b48970a6e1f", "commit_rev":
"65881de6c81cf3f33b0f2f5f8f4a19716ee93fdc"}

[commit-bot: I haz the power, 2017-04-26 15:23:39]

Description was changed from

==========
NetEq: Limit payload size for replacement audio input

With this fix, the size of the fake encoded payload is limited to 120
ms at 48000 samples/second.

BUG=webrtc:7467
==========

to

==========
NetEq: Limit payload size for replacement audio input

With this fix, the size of the fake encoded payload is limited to 120
ms at 48000 samples/second.

BUG=webrtc:7467

Review-Url: https://codereview.webrtc.org/2838353002
Cr-Commit-Position: refs/heads/master@{#17891}
Committed:
https://chromium.googlesource.com/external/webrtc/+/65881de6c81cf3f33b0f2f5f8...
==========

[commit-bot: I haz the power, 2017-04-26 15:23:40]

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