Resolve media/cast crypto crash, and add crash reporting.

Resolve media/cast crypto crash, and add crash reporting.

BUG=519022
Committed: https://crrev.com/ff618a8c4b0c79d87f0f367e76c828696ae950e3
Cr-Commit-Position: refs/heads/master@{#357027}

[miu, 2015-10-29 21:59:27]

miu@chromium.org changed reviewers:
+ xjz@chromium.org

[miu, 2015-10-29 21:59:27]

xjz: PTAL.

[miu, 2015-10-29 22:00:01]

Description was changed from

==========
Resolve media/cast crypto crash, and add crash reporting.

BUG=519022
==========

to

==========
Resolve media/cast crypto crash, and add crash reporting.

BUG=519022
==========

[miu, 2015-10-29 22:00:01]

miu@chromium.org changed reviewers:
+ rsesek@chromium.org

[miu, 2015-10-29 22:01:51]

rsesek: Need your OWNERS approval for temporary crash keys.  BTW--Do I need to
do anything special w/ the crash reporting infrastructure because of the new
key?  Also, I plan to merge this into M-47.

[Robert Sesek, 2015-10-29 22:07:18]

LGTM

No, the crash server will pick these up automatically and show them under the
Fields >Product Data section.

[xjz, 2015-10-30 00:24:33]

LGTM

Does this really happen? With no error the encoder shouldn't be able to produce
empty stream. There should be at least some header information.

[miu, 2015-10-30 01:18:47]

On 2015/10/30 00:24:33, xjz wrote:
> LGTM
> 
> Does this really happen? With no error the encoder shouldn't be able to
produce
> empty stream. There should be at least some header information.

That's what I'm trying to investigate here.  All I had in the crash report was a
stack trace, and it could have come from any of the various audio or video
encoders we use.  By looking at the code, I was able to rule out all the audio
encoders, but not any of the video encoders.  So, hopefully this new crash
reporting will expose the problem directly.

[miu, 2015-10-30 01:19:22]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2015-10-30 01:20:14]

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

[commit-bot: I haz the power, 2015-10-30 02:23:44]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-10-30 02:24:44]

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

[brucedawson, 2015-11-02 21:22:58]

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

[brucedawson, 2015-11-02 21:22:59]

The /analyze builder pointed out a VC++ specific problem with this change.

https://codereview.chromium.org/1417873007/diff/1/media/cast/sender/external_...
File media/cast/sender/external_video_encoder.cc (right):

https://codereview.chromium.org/1417873007/diff/1/media/cast/sender/external_...
media/cast/sender/external_video_encoder.cc:321: "%c/%c,id=%" PRIu32 ",rtp=%"
PRIu32 ",br=%d,q=%zu,act=%c,ref=%d",
Unfortunately this line of code uses a C99 feature that VC++ does not have yet.
VC++ 2015 supports %zu, but VC++ 2013 does not.

The portable fix to this is to use PRIuS (similar to PRIu32), or use a cast to
convert the size_t variable to a fixed type.

Using %zu in VC++ is undefined behavior. In practice it means that the %zu is
ignored and all subsequent values will be misapplied.

[miu, 2015-11-03 05:15:16]

On 2015/11/02 21:22:59, brucedawson wrote:
> The /analyze builder pointed out a VC++ specific problem with this change.
>
> ...
> 
> The portable fix to this is to use PRIuS (similar to PRIu32), or use a cast to
> convert the size_t variable to a fixed type.

Done: https://codereview.chromium.org/1411663012/