video_coding::PacketBuffer now group all H264 packets with the same timestamp into the same frame.

video_coding::PacketBuffer now group all H264 packets with the same timestamp into the same frame.

Since we can't know when a H264 frame really starts we instead group all packets together by timestamp when a frame seems to be complete (only in the case of H264).

BUG=webrtc:5514
Review-Url: https://codereview.webrtc.org/2675693002
Cr-Commit-Position: refs/heads/master@{#16419}
Committed: https://chromium.googlesource.com/external/webrtc/+/8c61924b5671b568a1953820961e4ec24e19e4c6

[philipel, 2017-02-02 14:59:42]

philipel@webrtc.org changed reviewers:
+ stefan@webrtc.org

[philipel, 2017-02-02 15:01:54]

Description was changed from

==========
win h264 fix.

BUG=
==========

to

==========
video_coding::PacketBuffer now group all H264 packets with the same timestamp
into the same frame.

Since we can't know when a H264 frame really starts we instead group all packets
together by timestamp when a frame seems to be complete (only in the case of
H264).

BUG=webrtc:5514
==========

[stefan-webrtc, 2017-02-02 15:19:00]

https://codereview.webrtc.org/2675693002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/packet_buffer.cc (right):

https://codereview.webrtc.org/2675693002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet_buffer.cc:228: if (is_h264 && start_index !=
static_cast<int>(index) &&
Can we have a short comment on what this is trying to achieve? It's a bit hard
for a reader to interpret.

Is it something like "Beginning of an h.264 frame has been reached if the
timestamp changes and the begin bit has been found on the previous packet."? 
Why is that not true for all codecs? And do we know that the previous packet had
a begin bit?

[philipel, 2017-02-02 15:47:37]

https://codereview.webrtc.org/2675693002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/packet_buffer.cc (right):

https://codereview.webrtc.org/2675693002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet_buffer.cc:228: if (is_h264 && start_index !=
static_cast<int>(index) &&
On 2017/02/02 15:19:00, stefan-webrtc wrote:
> Can we have a short comment on what this is trying to achieve? It's a bit hard
> for a reader to interpret.
> 
> Is it something like "Beginning of an h.264 frame has been reached if the
> timestamp changes and the begin bit has been found on the previous packet."? 
> Why is that not true for all codecs? And do we know that the previous packet
had
> a begin bit?

Added a comment about it.

[stefan-webrtc, 2017-02-02 15:50:07]

https://codereview.webrtc.org/2675693002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/packet_buffer.cc (right):

https://codereview.webrtc.org/2675693002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet_buffer.cc:232: // the PacketBuffer to hand
out incomplete frames.
Can't we still make sure that the frame begin bit is set on the first packet
that we actually find? Or can it actually be false for the first packet?

https://codereview.webrtc.org/2675693002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet_buffer.cc:237: // with packets of the same
timestamp.3
Remove 3

[stefan-webrtc, 2017-02-02 16:08:42]

lgtm with bug created to investigate the begin bit issue. refer to the bug in
the code.

[philipel, 2017-02-02 16:14:31]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2017-02-02 16:14:32]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2675693002/#ps80001 (title:
"Added link to bug")

[commit-bot: I haz the power, 2017-02-02 16:14:33]

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-02-02 16:51:29]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486052071759830,
"parent_rev": "1dffc62843286148288f482ef77d98342ba7d3d3", "commit_rev":
"8c61924b5671b568a1953820961e4ec24e19e4c6"}

[commit-bot: I haz the power, 2017-02-02 16:51:32]

Description was changed from

==========
video_coding::PacketBuffer now group all H264 packets with the same timestamp
into the same frame.

Since we can't know when a H264 frame really starts we instead group all packets
together by timestamp when a frame seems to be complete (only in the case of
H264).

BUG=webrtc:5514
==========

to

==========
video_coding::PacketBuffer now group all H264 packets with the same timestamp
into the same frame.

Since we can't know when a H264 frame really starts we instead group all packets
together by timestamp when a frame seems to be complete (only in the case of
H264).

BUG=webrtc:5514

Review-Url: https://codereview.webrtc.org/2675693002
Cr-Commit-Position: refs/heads/master@{#16419}
Committed:
https://chromium.googlesource.com/external/webrtc/+/8c61924b5671b568a19538209...
==========

[commit-bot: I haz the power, 2017-02-02 16:51:32]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/8c61924b5671b568a19538209...