WebRTC Code Reviews
Help | Chromium Project | Sign in
(14251)

Issue 13399004: H264 Packetization and Depacketization in WebRTC

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 5 months ago by Mallinath (Gone from WebRTC)
Modified:
4 years, 8 months ago
CC:
Niklas, hshi, webrtc-reviews_webrtc.org
Base URL:
http://webrtc.googlecode.com/svn/trunk
Visibility:
Public.

Description

This CL originally authored by kaiduanx@ and more details about it can be found here https://webrtc-codereview.appspot.com/11239004/ This CL removes the default population of H264 codec into codec database and creation of 264 encoder and decoder during initialization of VieChannel. H264 codec must be registered through RegisterExternalSendCodec and RegisterExternalReceiveCodec.

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Total comments: 1

Patch Set 6 : #

Patch Set 7 : Rebase #

Patch Set 8 : #

Total comments: 18

Patch Set 9 : #

Patch Set 10 : From Kaiduan, after updated with comments from Stefan/Randell. #

Total comments: 27
Unified diffs Side-by-side diffs Delta from patch set Stats (+609 lines, -146 lines) Patch
M build/common.gypi View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -0 lines 0 comments Download
M common_types.h View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -2 lines 0 comments Download
M modules/interface/module_common_types.h View 1 2 3 4 5 6 7 8 9 1 chunk +10 lines, -1 line 0 comments Download
A modules/rtp_rtcp/source/rtp_format_h264.h View 1 2 3 4 5 6 7 8 9 1 chunk +81 lines, -0 lines 2 comments Download
A modules/rtp_rtcp/source/rtp_format_h264.cc View 1 2 3 4 5 6 7 8 9 1 chunk +101 lines, -0 lines 6 comments Download
M modules/rtp_rtcp/source/rtp_payload_registry.cc View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M modules/rtp_rtcp/source/rtp_receiver_video.h View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -0 lines 0 comments Download
M modules/rtp_rtcp/source/rtp_receiver_video.cc View 1 2 3 4 5 6 7 8 9 3 chunks +73 lines, -0 lines 6 comments Download
M modules/rtp_rtcp/source/rtp_rtcp.gypi View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M modules/rtp_rtcp/source/rtp_sender_video.h View 1 2 3 4 5 6 7 8 9 1 chunk +9 lines, -0 lines 0 comments Download
M modules/rtp_rtcp/source/rtp_sender_video.cc View 1 2 3 4 5 6 7 8 9 4 chunks +98 lines, -42 lines 3 comments Download
A modules/video_coding/codecs/h264/include/h264.h View 1 2 3 4 5 6 7 8 9 1 chunk +35 lines, -0 lines 0 comments Download
M modules/video_coding/codecs/interface/video_codec_interface.h View 1 2 3 4 5 6 7 8 9 1 chunk +7 lines, -2 lines 1 comment Download
M modules/video_coding/main/source/codec_database.cc View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M modules/video_coding/main/source/encoded_frame.cc View 1 2 3 4 5 6 7 8 9 1 chunk +42 lines, -42 lines 0 comments Download
M modules/video_coding/main/source/internal_defines.h View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -2 lines 1 comment Download
M modules/video_coding/main/source/packet.cc View 1 2 3 4 5 6 7 8 9 2 chunks +59 lines, -26 lines 5 comments Download
M modules/video_coding/main/source/session_info.cc View 1 2 3 4 5 6 7 8 9 2 chunks +74 lines, -29 lines 3 comments Download
M video_engine/test/auto_test/source/vie_autotest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download
M video_engine/vie_codec_impl.cc View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
Trybot results: Sign in to try more bots

Messages

Total messages: 25 (0 generated)
Mallinath (Gone from WebRTC)
5 years, 5 months ago (2014-04-29 16:53:07 UTC) #1
Mallinath (Gone from WebRTC)
https://webrtc-codereview.appspot.com/13399004/diff/50056/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc File webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc (right): https://webrtc-codereview.appspot.com/13399004/diff/50056/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc#newcode250 webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc:250: // TODO(mallinath): Check with Stefan/Magnus about the timestamp and ...
5 years, 5 months ago (2014-04-29 16:55:17 UTC) #2
Mallinath (Gone from WebRTC)
Adding @kaiduanx to the reviewers list.
5 years, 5 months ago (2014-05-02 21:12:59 UTC) #3
Mallinath (Gone from WebRTC)
Adding @jesup to the reviewers list.
5 years, 5 months ago (2014-05-07 21:55:53 UTC) #4
jesup
It would seem that an H264 entry in VideoCodecUnion to carry SDP-negotiated values is needed, ...
5 years, 5 months ago (2014-05-11 18:31:28 UTC) #5
Stefan
https://webrtc-codereview.appspot.com/13399004/diff/240001/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc File webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc (right): https://webrtc-codereview.appspot.com/13399004/diff/240001/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc#newcode261 webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc:261: rtp_header->header.timestamp -= 50; +1. Will the jitter buffer not ...
5 years, 5 months ago (2014-05-11 19:14:58 UTC) #6
kaiduanx
https://webrtc-codereview.appspot.com/13399004/diff/240001/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc File webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc (right): https://webrtc-codereview.appspot.com/13399004/diff/240001/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc#newcode261 webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc:261: rtp_header->header.timestamp -= 50; SPS/PPS/IDR shares the same time stamp, ...
5 years, 5 months ago (2014-05-12 15:08:33 UTC) #7
Stefan
https://webrtc-codereview.appspot.com/13399004/diff/240001/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc File webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc (right): https://webrtc-codereview.appspot.com/13399004/diff/240001/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc#newcode261 webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc:261: rtp_header->header.timestamp -= 50; I think the problem is how ...
5 years, 5 months ago (2014-05-12 15:17:42 UTC) #8
kaiduanx
https://webrtc-codereview.appspot.com/13399004/diff/240001/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc File webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc (right): https://webrtc-codereview.appspot.com/13399004/diff/240001/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc#newcode261 webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc:261: rtp_header->header.timestamp -= 50; Stefan, I am not sure I ...
5 years, 5 months ago (2014-05-12 15:31:08 UTC) #9
Stefan
https://webrtc-codereview.appspot.com/13399004/diff/240001/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc File webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc (right): https://webrtc-codereview.appspot.com/13399004/diff/240001/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc#newcode261 webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc:261: rtp_header->header.timestamp -= 50; Yes, the point is that it ...
5 years, 5 months ago (2014-05-12 20:47:17 UTC) #10
kaiduanx
https://webrtc-codereview.appspot.com/13399004/diff/240001/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc File webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc (right): https://webrtc-codereview.appspot.com/13399004/diff/240001/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc#newcode261 webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc:261: rtp_header->header.timestamp -= 50; How about the following solution? 1) ...
5 years, 5 months ago (2014-05-12 21:41:12 UTC) #11
Stefan
https://webrtc-codereview.appspot.com/13399004/diff/240001/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc File webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc (right): https://webrtc-codereview.appspot.com/13399004/diff/240001/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc#newcode261 webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc:261: rtp_header->header.timestamp -= 50; I think that since we're only ...
5 years, 5 months ago (2014-05-12 23:00:29 UTC) #12
kaiduanx
https://webrtc-codereview.appspot.com/13399004/diff/240001/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc File webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc (right): https://webrtc-codereview.appspot.com/13399004/diff/240001/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc#newcode261 webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc:261: rtp_header->header.timestamp -= 50; Stefan, I assume you would like ...
5 years, 5 months ago (2014-05-13 19:52:24 UTC) #13
Stefan
https://webrtc-codereview.appspot.com/13399004/diff/240001/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc File webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc (right): https://webrtc-codereview.appspot.com/13399004/diff/240001/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc#newcode261 webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc:261: rtp_header->header.timestamp -= 50; I'd prefer to not have as ...
5 years, 5 months ago (2014-05-13 21:31:32 UTC) #14
kaiduanx
https://webrtc-codereview.appspot.com/13399004/diff/240001/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc File webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc (right): https://webrtc-codereview.appspot.com/13399004/diff/240001/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc#newcode261 webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc:261: rtp_header->header.timestamp -= 50; Adding codec specific code for H.264 ...
5 years, 5 months ago (2014-05-14 14:04:40 UTC) #15
Mallinath (Gone from WebRTC)
@kaiduanx, can you upload your changes in your original CL, or on top of this ...
5 years, 5 months ago (2014-05-14 18:42:33 UTC) #16
Mallinath (Gone from WebRTC)
From Kaiduan, after updated with comments from Stefan/Randell.
5 years, 5 months ago (2014-05-16 16:58:12 UTC) #17
Mallinath (Gone from WebRTC)
On 2014/05/16 16:58:12, mallinath1 wrote: > From Kaiduan, after updated with comments from Stefan/Randell. Stefan, ...
5 years, 5 months ago (2014-05-16 17:33:18 UTC) #18
jesup
On 2014/05/14 14:04:40, kaiduanx wrote: > https://webrtc-codereview.appspot.com/13399004/diff/240001/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc > File webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc (right): > > https://webrtc-codereview.appspot.com/13399004/diff/240001/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc#newcode261 > ...
5 years, 5 months ago (2014-05-18 13:21:17 UTC) #19
jesup
Please re-read my original comments and make sure all are addressed or purposely not addressed ...
5 years, 5 months ago (2014-05-18 16:43:47 UTC) #20
jesup
I have this patchset imported into Mozilla and working. You can see the modifications I ...
5 years, 5 months ago (2014-05-19 08:45:14 UTC) #21
kaiduanx
From Jesup, "b) clean up the entire first-packet/SPS/PPS/etc stuff - treat SPS/PPS as simple single-nalu's. ...
5 years, 5 months ago (2014-05-19 21:18:41 UTC) #22
jesup
On 2014/05/19 08:45:14, jesup wrote: > I have this patchset imported into Mozilla and working. ...
5 years, 5 months ago (2014-05-20 07:20:02 UTC) #23
kaiduanx
On 2014/05/20 07:20:02, jesup wrote: > On 2014/05/19 08:45:14, jesup wrote: > > I have ...
5 years, 5 months ago (2014-05-20 13:51:26 UTC) #24
kaiduanx
5 years, 5 months ago (2014-05-20 18:36:06 UTC) #25
On 2014/05/20 13:51:26, kaiduanx wrote:
> On 2014/05/20 07:20:02, jesup wrote:
> > On 2014/05/19 08:45:14, jesup wrote:
> > > I have this patchset imported into Mozilla and working.
> > > 
> > > You can see the modifications I made in bug 985254 there.  (Bug 986253 has
> the
> > > SDP/etc mods needed)
> > > 
> > > Major ones:
> > > 
> > > a) Add H264-specific codec data to support negotiation of params (mostly
> still
> > > unused in my patchset)
> > >
> >
>
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=985254&attachment=...
> > > 
> > > b) clean up the entire first-packet/SPS/PPS/etc stuff - treat SPS/PPS as
> > simple
> > > single-nalu's.  This made the code noticeably cleaner, but requires one
> (IMHO
> > > correct) change, which is to make the IsOlder*() funcs in the jitter
buffer
> > use
> > > IsNewerOrSameTimestamp() instead of IsNewerTimestamp().  An alternative
> would
> > be
> > > to add a non-frame-coding frametype for SPS/PPS/SEI/etc NALs, and use
> > > NewerOrSame only for those.
> > >
> >
>
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=985254&attachment=...
> > 
> > Uploaded my cleaned-up patchset to Issue 21499004
> > (https://webrtc-codereview.appspot.com/21499004/).  Couldn't figure out how
to
> > add it as a new patch here, which would be preferred.
> 
> Jesup,
> 
> You missed files rtp_format_h264.h/rtp_format_h264.cc in
> https://webrtc-codereview.appspot.com/21499004/
> 
> You forgot to run svn add rtp_format_h264.h/rtp_format_h264.cc before creating
> change using gcl change.
> 
> In packet.cc of 21499004,
> 
> uint8_t nal_type = videoHeader.codecHeader.H264.nalu_header & RtpFormatH26
> 4::kH264NAL_TypeMask;
> 
> nal_type is not used.
> 
> If you want to add the patch set in 21499004 to here, you can create a tar
ball
> of whole webrtc source code tree such as webrtc-jseup.tar.gz, and send
> webrtc-jseup.tar.gz to Mallinath because Mallinath is the owner of this issue
> and only he can add new patch set.


Hi all,

Please see the comment in 21499004, the patch listed there fails to make video
call on Chrome, the log is attached there too.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 245c2c2-tainted