Issue 1306813009: H.264 video codec support using OpenH264/FFmpeg

hbos

Patchset #4 (id:60001) has been deleted

5 years, 3 months ago (2015-09-11 09:27:04 UTC) #1

Niklas Enbom

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

5 years, 3 months ago (2015-09-14 22:33:14 UTC) #4

Niklas Enbom

https://codereview.webrtc.org/1306813009/diff/120001/webrtc/modules/video_coding/codecs/h264/h264.gypi File webrtc/modules/video_coding/codecs/h264/h264.gypi (right): https://codereview.webrtc.org/1306813009/diff/120001/webrtc/modules/video_coding/codecs/h264/h264.gypi#newcode34 webrtc/modules/video_coding/codecs/h264/h264.gypi:34: '-L../../webrtc/modules/video_coding/codecs/h264/openh264', FYI does not work when building chrome https://codereview.webrtc.org/1306813009/diff/120001/webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc ...

5 years, 3 months ago (2015-09-14 22:33:15 UTC) #5

hbos

hbos@webrtc.org changed reviewers: + holmer@chromium.org, mflodman@webrtc.org, noahric@chromium.org

5 years, 3 months ago (2015-09-15 15:25:33 UTC) #9

hbos

PTAL, H264-loving reviewers. Not landable yet - when we have sorted out if we are ...

5 years, 3 months ago (2015-09-15 15:25:34 UTC) #10

noahric

First pass, sorry for the delay! https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right): https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc#newcode35 webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:35: case videoFrameTypeIDR: IDR ...

5 years, 3 months ago (2015-09-25 00:11:32 UTC) #12

First pass, sorry for the delay!

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right):

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:35: case
videoFrameTypeIDR:
IDR == kKeyFrame, right?

Also SPS/PPS are treated as keyframes in the rest of webrtc. It's unclear if/how
those are handled here; does OpenH264 always put SPS/PPS/IDR in the same frame
and call that IDR?

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:50: // with the
four-byte NAL header {0,0,0,1}. To save bytes, the NAL headers are
For clarity, I would avoid calling those NAL headers. They are start codes or
start code prefixes. NAL headers generally refer to things like NAL
packetization headers.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:50: // with the
four-byte NAL header {0,0,0,1}. To save bytes, the NAL headers are
It's up to you, but it's probably not worth stripping the start code prefixes.
They should be insignificant in terms of relative size (other than SPS/PPS), and
it lets you turn things into basically a reserve size + memcpy.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:78: // Encoded data
> unencoded data, wtf? Allocate required bytes.
Consider logging this. You're right, it's pretty WTF :)

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:82: delete[]
encoded_image->_buffer;
Consider storing the buffer separately and with scoped_ptr, for safety.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:184: if
(&codec_settings_ != codec_settings)
This won't ever be false, will it?

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:192: // -
codec_settings->codecSpecific.H264.keyFrameInterval
Assuming it's set, you don't really want to skip it.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:214:
init_params.fMaxFrameRate = static_cast<float>(codec_settings_.maxFramerate);
Not sure how to set these, but there are few others I'd expect that need to be
set:
uiIntraPeriod - keyframe period. Should be far between, if periodic keyframes
aren't enabled.
iEntropyCodingModeFlag - the recent h264 qp parser doesn't understand cabac, so
make sure this is false
iNumRefFrame - 1; maybe inferred from camera_video_real_time?
uiProfileIdc - baseline, for fastest processing on decoders of various platforms
<something that disallows B frames> - maybe realtime implies this as well, or
maybe iNumRefFrame == 1 forces it

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:226: &trace_level);
What's the default? If nothing, then consider setting WELS_LOG_WARNING in an
else block.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:287: if
(codec_settings_.width != frame.width() ||
That won't be right if simulcast is used, will it? Or does some other component
scale the frame before it arrives here?

That's hypothetical, of course, but it points at a more general point: I don't
think the encoder should try to match the input, it should match whatever it's
configured to produce.

(Also, it looks like this value isn't used outside of init, so it's kinda
confusing to have it modified here)

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:302:
&target_bitrate);
Why not just set these in SetRates?

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:326: if
(openh264_encoder_->EncodeFrame(&picture, &info) != 0) {
It's probably worth capturing the return and logging it.

stefan-webrtc

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

5 years, 2 months ago (2015-09-28 11:19:00 UTC) #15

stefan-webrtc

https://codereview.webrtc.org/1306813009/diff/250001/talk/media/webrtc/webrtcvideoengine2_unittest.cc File talk/media/webrtc/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/1306813009/diff/250001/talk/media/webrtc/webrtcvideoengine2_unittest.cc#newcode670 talk/media/webrtc/webrtcvideoengine2_unittest.cc:670: // Test external codec with be added to the ...

5 years, 2 months ago (2015-09-28 11:19:03 UTC) #16

hbos

Addressed noahric's comments. (Stefan just added comments a few minutes ago too, will address these ...

5 years, 2 months ago (2015-09-28 11:33:20 UTC) #17

Addressed noahric's comments. (Stefan just added comments a few minutes ago too,
will address these next.)

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right):

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:35: case
videoFrameTypeIDR:
On 2015/09/25 00:11:31, noahric wrote:
> IDR == kKeyFrame, right?
> 
> Also SPS/PPS are treated as keyframes in the rest of webrtc. It's unclear
if/how
> those are handled here; does OpenH264 always put SPS/PPS/IDR in the same frame
> and call that IDR?

An I-frame is a key frame, and all IDR-frames are I-frames (but not all I-frames
are IDR-frames)? I'm not sure about the subtle differences, but I changed so
that IDR and I are both treated as kKeyFrame.

As for the SPS/PPS question: I don't know. Does any of the other reviewers know?

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:50: // with the
four-byte NAL header {0,0,0,1}. To save bytes, the NAL headers are
On 2015/09/25 00:11:31, noahric wrote:
> It's up to you, but it's probably not worth stripping the start code prefixes.
> They should be insignificant in terms of relative size (other than SPS/PPS),
and
> it lets you turn things into basically a reserve size + memcpy.

   First comment:

There are assumptions about the inclusion/exclusion of these headers in other
code that I am not very familiar with. I had to do this for it work without
changing other H264 packetization code.

But it's an interesting point - do we want to do this? Because we do it I had to
add the RTPDefragmentization code which is used quiet hackily @
videoprocessor.cc:280. All other encoders produce data that can be directly fed
to the decoder, now with the H264 case we have to manipulate the byte stream
before feeding it to the decoder (something we have to do explicitly in local
unittests or that is done for us by the "depacketizer stuff" when receiving over
network/loopback).

I don't know how this works if communicating cross browsers. Would firefox also
strip the headers? If not, would we include double headers on the receive side?
I'm guessing there's a good reason for doing it.
stefan and mflodman might know more about why we do this and if we want to keep
doing it.

   Second comment:

OK, updated comment. Still calling them NAL units, is this correct?

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:78: // Encoded data
> unencoded data, wtf? Allocate required bytes.
On 2015/09/25 00:11:31, noahric wrote:
> Consider logging this. You're right, it's pretty WTF :)

Done.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:82: delete[]
encoded_image->_buffer;
On 2015/09/25 00:11:31, noahric wrote:
> Consider storing the buffer separately and with scoped_ptr, for safety.

Done.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:184: if
(&codec_settings_ != codec_settings)
On 2015/09/25 00:11:31, noahric wrote:
> This won't ever be false, will it?

You're right. Fixed.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:192: // -
codec_settings->codecSpecific.H264.keyFrameInterval
On 2015/09/25 00:11:31, noahric wrote:
> Assuming it's set, you don't really want to skip it.

I don't know what to do with these. I believe the iOS implementation use them.
I'll try to dig some more before a future patch set.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:214:
init_params.fMaxFrameRate = static_cast<float>(codec_settings_.maxFramerate);
On 2015/09/25 00:11:31, noahric wrote:
> Not sure how to set these, but there are few others I'd expect that need to be
> set:
> uiIntraPeriod - keyframe period. Should be far between, if periodic keyframes
> aren't enabled.
> iEntropyCodingModeFlag - the recent h264 qp parser doesn't understand cabac,
so
> make sure this is false
> iNumRefFrame - 1; maybe inferred from camera_video_real_time?
> uiProfileIdc - baseline, for fastest processing on decoders of various
platforms
> <something that disallows B frames> - maybe realtime implies this as well, or
> maybe iNumRefFrame == 1 forces it

Those parameters are available if SEncParamExt is used instead of SEncParamBase
and they are, amongst other parameters, set to default values with
GetDefaultParams. If the default values are used for the extra params not in
SEncParamBase I don't know whether or not there is a difference between using
SEncParamBase and SEncParamExt. Switching to SEncParamExt to make more
parameters available but not sure if I should explicitly set all parameters or
rely on GetDefaultParams.

Some FYI information about these parameters. They might say more to the
reviewers than me, much of it is like a different language to me.

These are comments from the source code about these properties from
codec_app_def.h/SEncParamExt and param_cvc.h/FillDefault.
- uiIntraPeriod: "period of Intra frame", "multiple of GOP size as desired"
- iEntropyCodingModeFlag: "0:CAVLC  1:CABAC"
- iNumRefFrame: "number of reference frame used"
- uiProfileIdc: "value of profile IDC (PRO_UNKNOWN for auto-detection)"

This is how the default values are set:
https://github.com/cisco/openh264/blob/master/codec/encoder/core/inc/param_sv...

Here's an example of existing chromium code setting these parameters:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg...

iEntropyCodingModeFlag and uiProfileIdc are set appropriately by default. Do you
want me to explicitly set these anyway to make it explicit and in case the
openh264 default values change in the future or leave as-is after setting
defaults?
By default, iNumRefFrame = AUTO_REF_PIC_COUNT, which is = -1 with comment:
"encoder selects the number of reference frame automatically". Should I really
set this to 1 and not use the automatic setting?
As for uiIntraPeriod, I have no idea what to set this to. It defaults to 0.

Switched to SEncParamExt but relying on a lot of defaults atm. Had to set
sSpatialLayers[0] values for it to work.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:226: &trace_level);
On 2015/09/25 00:11:31, noahric wrote:
> What's the default? If nothing, then consider setting WELS_LOG_WARNING in an
> else block.

It has a default value it uses, WELS_LOG_DEFAULT (= WELS_LOG_WARNING), no need
to set.
Because this is a constant I moved the SetOption to init.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:287: if
(codec_settings_.width != frame.width() ||
On 2015/09/25 00:11:31, noahric wrote:
> That won't be right if simulcast is used, will it? Or does some other
component
> scale the frame before it arrives here?
> 
> That's hypothetical, of course, but it points at a more general point: I don't
> think the encoder should try to match the input, it should match whatever it's
> configured to produce.
> 
> (Also, it looks like this value isn't used outside of init, so it's kinda
> confusing to have it modified here)

Sorry, I'm not sure I follow your simulcast comment. Won't be right if simulcast
is used? What do you mean? Whatever the path we just encode the frame, right?
I'm thinking scaling is someone else's problem?

I proceeded to encode with the new frame size and updated |codec_settings_| to
reflect the latest settings, even though |codec_settings_.width,height| was not
actually used (unlike |codec_settings_.targetBitrate,maxFramerate|).

But as you say perhaps we want to fail if encoding is attempted with the wrong
frame size. I'm not sure what is correct, it looks like VP8 adapts and VP9
crashes (DCHECK).
I changed the code to return WEBRTC_VIDEO_CODEC_ERR_SIZE if there is a size
mismatch.

I could remove |codec_settings_| completely now but it might be nice to have for
debugging?

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:302:
&target_bitrate);
On 2015/09/25 00:11:31, noahric wrote:
> Why not just set these in SetRates?

Done.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:326: if
(openh264_encoder_->EncodeFrame(&picture, &info) != 0) {
On 2015/09/25 00:11:30, noahric wrote:
> It's probably worth capturing the return and logging it.

Done.

stefan-webrtc

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right): https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc#newcode50 webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:50: // with the four-byte NAL header {0,0,0,1}. To save ...

5 years, 2 months ago (2015-09-28 11:53:55 UTC) #18

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right):

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:50: // with the
four-byte NAL header {0,0,0,1}. To save bytes, the NAL headers are
On 2015/09/28 11:33:20, hbos wrote:
> On 2015/09/25 00:11:31, noahric wrote:
> > It's up to you, but it's probably not worth stripping the start code
prefixes.
> > They should be insignificant in terms of relative size (other than SPS/PPS),
> and
> > it lets you turn things into basically a reserve size + memcpy.
> 
>    First comment:
> 
> There are assumptions about the inclusion/exclusion of these headers in other
> code that I am not very familiar with. I had to do this for it work without
> changing other H264 packetization code.
> 
> But it's an interesting point - do we want to do this? Because we do it I had
to
> add the RTPDefragmentization code which is used quiet hackily @
> videoprocessor.cc:280. All other encoders produce data that can be directly
fed
> to the decoder, now with the H264 case we have to manipulate the byte stream
> before feeding it to the decoder (something we have to do explicitly in local
> unittests or that is done for us by the "depacketizer stuff" when receiving
over
> network/loopback).

The RTP format for H264 specifies that start codes shouldn't be included in the
packets. I think Noah mainly suggested that it doesn't matter if you copy the
startcodes here, you can simply skip them when setting the fragmentation header
below.

> 
> I don't know how this works if communicating cross browsers. Would firefox
also
> strip the headers? If not, would we include double headers on the receive
side?
> I'm guessing there's a good reason for doing it.
> stefan and mflodman might know more about why we do this and if we want to
keep
> doing it.
> 
>    Second comment:
> 
> OK, updated comment. Still calling them NAL units, is this correct?

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:214:
init_params.fMaxFrameRate = static_cast<float>(codec_settings_.maxFramerate);
On 2015/09/28 11:33:20, hbos wrote:
> On 2015/09/25 00:11:31, noahric wrote:
> > Not sure how to set these, but there are few others I'd expect that need to
be
> > set:
> > uiIntraPeriod - keyframe period. Should be far between, if periodic
keyframes
> > aren't enabled.
> > iEntropyCodingModeFlag - the recent h264 qp parser doesn't understand cabac,
> so
> > make sure this is false
> > iNumRefFrame - 1; maybe inferred from camera_video_real_time?
> > uiProfileIdc - baseline, for fastest processing on decoders of various
> platforms
> > <something that disallows B frames> - maybe realtime implies this as well,
or
> > maybe iNumRefFrame == 1 forces it
> 
> Those parameters are available if SEncParamExt is used instead of
SEncParamBase
> and they are, amongst other parameters, set to default values with
> GetDefaultParams. If the default values are used for the extra params not in
> SEncParamBase I don't know whether or not there is a difference between using
> SEncParamBase and SEncParamExt. Switching to SEncParamExt to make more
> parameters available but not sure if I should explicitly set all parameters or
> rely on GetDefaultParams.
> 
> 
> Some FYI information about these parameters. They might say more to the
> reviewers than me, much of it is like a different language to me.
> 
> These are comments from the source code about these properties from
> codec_app_def.h/SEncParamExt and param_cvc.h/FillDefault.
> - uiIntraPeriod: "period of Intra frame", "multiple of GOP size as desired"
> - iEntropyCodingModeFlag: "0:CAVLC  1:CABAC"
> - iNumRefFrame: "number of reference frame used"
> - uiProfileIdc: "value of profile IDC (PRO_UNKNOWN for auto-detection)"
> 
> This is how the default values are set:
>
https://github.com/cisco/openh264/blob/master/codec/encoder/core/inc/param_sv...
> 
> Here's an example of existing chromium code setting these parameters:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg...
> 
> iEntropyCodingModeFlag and uiProfileIdc are set appropriately by default. Do
you
> want me to explicitly set these anyway to make it explicit and in case the
> openh264 default values change in the future or leave as-is after setting
> defaults?
> By default, iNumRefFrame = AUTO_REF_PIC_COUNT, which is = -1 with comment:
> "encoder selects the number of reference frame automatically". Should I really
> set this to 1 and not use the automatic setting?

I think automatic is better. There is no reason that I know of where we don't
want to use multiple reference frames if possible. Or are you thinking of CPU
issues, Noah?

> As for uiIntraPeriod, I have no idea what to set this to. It defaults to 0.

0 is no intra frame. That is good for the normal case, but if
codec_settings->codecSpecific.H264.keyFrameInterval is set we should use that to
set uiIntraPeriod, I believe.

> 
> 
> Switched to SEncParamExt but relying on a lot of defaults atm. Had to set
> sSpatialLayers[0] values for it to work.

Makes sense, which means you are configuring the base spatial layer (which is
the only layer we use).

hbos

Please take another look https://codereview.webrtc.org/1306813009/diff/250001/talk/media/webrtc/webrtcvideoengine2_unittest.cc File talk/media/webrtc/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/1306813009/diff/250001/talk/media/webrtc/webrtcvideoengine2_unittest.cc#newcode670 talk/media/webrtc/webrtcvideoengine2_unittest.cc:670: // Test external codec with ...

5 years, 2 months ago (2015-09-30 15:35:19 UTC) #19

Please take another look

https://codereview.webrtc.org/1306813009/diff/250001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/1306813009/diff/250001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvideoengine2_unittest.cc:670: // Test external codec
with be added to the end of the supported codec list.
On 2015/09/28 11:19:01, stefan-webrtc (holmer) wrote:
> s/with/will

Changed sentence.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/build/common.gypi
File webrtc/build/common.gypi (right):

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/build/common.gypi...
webrtc/build/common.gypi:133: # LICENCING FEES.
On 2015/09/28 11:19:01, stefan-webrtc (holmer) wrote:
> LICENSING

Done.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264.cc (right):

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264.cc:55: return
!IsH264CodecSupportedObjC();
On 2015/09/28 11:19:01, stefan-webrtc (holmer) wrote:
> This seems a bit strange to me. Shouldn't we always first check if H264 HW is
> supported, and if not, check if OpenH264 is supported? We shouldn't say
OpenH264
> isn't supported just because we have HW. For instance, we might want to fall
> back to OpenH264 if the HW fails.

You're right. If we are able to support both HW and SW it should be possible to
control and support both in code.

Right now there's just a pair of H264[En/De]coder::Create functions and no
concept of choosing between multiple different implementations of the same
codec. It's implementation-agnostic and I'm not sure how easy/hard it is to fix
this (where to decide/prioritize?).

Can I add a TODO and address this in a separate CL?

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264.gypi (right):

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264.gypi:29: # TODO(hbos): Add OpenH264
to third_party and update dependencies.
On 2015/09/28 11:19:01, stefan-webrtc (holmer) wrote:
> Are we fixing this TODO before committing this CL?

Yes. I want to be able to build from source and run the trybots. But this may be
difficult and I wanted to kick off the review process before that to be able to
work in parallel.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right):

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:28: const bool
OPENH264_DECODER_LOGGING = false;
On 2015/09/28 11:19:01, stefan-webrtc (holmer) wrote:
> I think it's more common to use kOpenH264DecodeLogging in WebRTC for compile
> time constants.

Done.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:29: }  // anonymous
namespace
On 2015/09/28 11:19:01, stefan-webrtc (holmer) wrote:
> // namespace

Done.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:51:
DCHECK(!openh264_decoder_);
On 2015/09/28 11:19:02, stefan-webrtc (holmer) wrote:
> I think you have to rebase since DCHECK is now RTC_DCHECK.

Done.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:55: // Failed to
create decoder.
On 2015/09/28 11:19:01, stefan-webrtc (holmer) wrote:
> This comment isn't needed since the log below says the same thing.

Done.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:66:
init_params.uiCpuLoad = 0;
On 2015/09/28 11:19:02, stefan-webrtc (holmer) wrote:
> Looks like this is unused by OpenH264, but does it make sense to set it to 0?
> cpu load = 0 seems like bad choice.

The decoder example on github just clears all the members and then proceeds to
set the desired members, meaning uiCpuLoad is left as 0 by the looks of it. Not
knowing what to set it to I did the same thing (except I listed all the members
and set them explicitly after the initial memset 0, for clarity). Zero may very
well be interpreted as unspecified.
If not we can change this in the future if a later openh264 version gives us
problems.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:159:
CHECK_EQ(decode_ret, 0);
On 2015/09/28 11:19:01, stefan-webrtc (holmer) wrote:
> Should this be a DCHECK?

Done.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right):

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:30: const
EVideoFrameType& type) {
On 2015/09/28 11:19:02, stefan-webrtc (holmer) wrote:
> You should be able to pass this by value since it's an enum?

Done.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:34: return
kSkipFrame;
On 2015/09/28 11:19:02, stefan-webrtc (holmer) wrote:
> I think we can send videoFrameTypeSkip as a kDeltaFrame. kSkipFrame has been
> used to notify the upper layers that the frame was dropped completely by the
> encoder, which isn't the case for an h.264 skip frame, I think?

I don't think the upper layers are notified at all unless the encode was
successful? |encoded_image_callback_| is only used if we encoded something. I
changed videoFrameTypeSkip to kDeltaFrame, but I'm not sure this matters or if
it makes sense for a "skip" to be called "delta", sounds dubious.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:35: case
videoFrameTypeIDR:
On 2015/09/28 11:19:02, stefan-webrtc (holmer) wrote:
> On 2015/09/25 00:11:31, noahric wrote:
> > IDR == kKeyFrame, right?
> > 
> > Also SPS/PPS are treated as keyframes in the rest of webrtc. It's unclear
> if/how
> > those are handled here; does OpenH264 always put SPS/PPS/IDR in the same
frame
> > and call that IDR?
> 
> Agree. videoFrameTypeI should however probably be a kDeltaFrame since it
doesn't
> necessarily unset all reference buffers.

@stefan: Ok, kDeltaFrame for videoFrameTypeI it is. (And kKeyFrame for
videoFrameTypeIDR)

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:50: // with the
four-byte NAL header {0,0,0,1}. To save bytes, the NAL headers are
On 2015/09/28 11:53:55, stefan-webrtc (holmer) wrote:
> On 2015/09/28 11:33:20, hbos wrote:
> > On 2015/09/25 00:11:31, noahric wrote:
> > > It's up to you, but it's probably not worth stripping the start code
> prefixes.
> > > They should be insignificant in terms of relative size (other than
SPS/PPS),
> > and
> > > it lets you turn things into basically a reserve size + memcpy.
> > 
> >    First comment:
> > 
> > There are assumptions about the inclusion/exclusion of these headers in
other
> > code that I am not very familiar with. I had to do this for it work without
> > changing other H264 packetization code.
> > 
> > But it's an interesting point - do we want to do this? Because we do it I
had
> to
> > add the RTPDefragmentization code which is used quiet hackily @
> > videoprocessor.cc:280. All other encoders produce data that can be directly
> fed
> > to the decoder, now with the H264 case we have to manipulate the byte stream
> > before feeding it to the decoder (something we have to do explicitly in
local
> > unittests or that is done for us by the "depacketizer stuff" when receiving
> over
> > network/loopback).
> 
> The RTP format for H264 specifies that start codes shouldn't be included in
the
> packets. I think Noah mainly suggested that it doesn't matter if you copy the
> startcodes here, you can simply skip them when setting the fragmentation
header
> below.
> 
> > 
> > I don't know how this works if communicating cross browsers. Would firefox
> also
> > strip the headers? If not, would we include double headers on the receive
> side?
> > I'm guessing there's a good reason for doing it.
> > stefan and mflodman might know more about why we do this and if we want to
> keep
> > doing it.
> > 
> >    Second comment:
> > 
> > OK, updated comment. Still calling them NAL units, is this correct?
> 

Oh I see, I misunderstood. I didn't considered not stripping them for some
reason, but that is great. Then we don't need RtpDefragmentize anymore, encoded
data can be fed directly to the decoder! :)

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:103:
layerInfo.pBsBuf + iLayerLen + 4,
On 2015/09/28 11:19:02, stefan-webrtc (holmer) wrote:
> Do you know if these layers are stored in one chunk of memory? In that case
you
> could copy all at once and just set the fragmentation header to ignore the
start
> codes.

Done.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:116: off +=
frags[i];
On 2015/09/28 11:19:02, stefan-webrtc (holmer) wrote:
> frags -> fragments
> off -> offset

Done.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:120: void
H264EncoderImpl::RTPDefragmentize(
On 2015/09/28 11:19:02, stefan-webrtc (holmer) wrote:
> Is this a test function? In that case I would move it to the test code.

(Removed, no longer needed due to including start codes.)

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:143: size_t
H264EncoderImpl::RTPDefragmentizeBufferLengthWithNAL(
On 2015/09/28 11:19:02, stefan-webrtc (holmer) wrote:
> Same with this.

(Removed, no longer needed due to including start codes.)

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:191: // -
codec_settings->codecSpecific.H264.frameDroppingOn
On 2015/09/28 11:19:02, stefan-webrtc (holmer) wrote:
> Have you taken a look to see if it's possible to configure something like
this?
> It's typically available in rate control settings.
> 
> See bEnableFrameSkip.

Done.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:192: // -
codec_settings->codecSpecific.H264.keyFrameInterval
On 2015/09/28 11:33:20, hbos wrote:
> On 2015/09/25 00:11:31, noahric wrote:
> > Assuming it's set, you don't really want to skip it.
> 
> I don't know what to do with these. I believe the iOS implementation use them.
> I'll try to dig some more before a future patch set.

Using them both now to set SEncParamExt's bEnableFrameSkip and uiIntraPeriod.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:198: // in
InitializeExt. We use SEncParamBase/Initialize.
On 2015/09/28 11:19:02, stefan-webrtc (holmer) wrote:
> Is there a reason why we use those?
> 
> I think we should consider using SEncParamExt as it allows for some more
> settings which could be useful, for instance:
> 
> iMultipleThreadIdc to enable multi-threading. May require slicing set to
> SM_AUTO_SLICE, but probably makes sense to do for HD resolutions.
> 
> bEnableDenoise to enable denoising? (not sure we want this, but we do have it
> for vp8 and vp9. It depends on its quality and complexity though, so I suggest
> ignoring it for now)
> 
> uiMaxNalSize = 0
> 
> 
> We can probably consider some of these and even more options down the line
> though. But it could make sense to look at enabling multi-threading at least?

Using SEncParamExt now. iMultileThreadIdc set to auto and
sSpatialLayers[0].sSliceCfg.uiSliceMode to SM_AUTO_SLICE.
Setting uiMaxNalSize = 0 too even though it is the default. What does 0 mean?

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:213:
init_params.iRCMode = RC_QUALITY_MODE;
On 2015/09/28 11:19:02, stefan-webrtc (holmer) wrote:
> I suspect a better choice would be RC_BITRATE_MODE or RC_TIMESTAMP_MODE. I
think
> RC_TIMESTAMP_MODE would make most sense since then we can pass a timestamp in
> milliseconds with each frame and the rate controller will try to use as many
> bits as the interval to the previous frame allows for.

I've noticed that you get really laggy video if the timestamps are not correct
(e.g. forgetting to convert from 90kHz timestamps to ms). VideoProcessorImpl
uses incorrect timestamp values (it uses frame count as timestamp) so at least
one test fails if I use RC_TIMESTAMP_MODE.

Switched to RC_BITRATE_MODE and added a TODO to fix this in a later CL if we
want to have RC_TIMESTAMP_MODE. I think RC_BITRATE_MODE is less prone to errors
which might be good for the initial CL.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:214:
init_params.fMaxFrameRate = static_cast<float>(codec_settings_.maxFramerate);
On 2015/09/28 11:53:55, stefan-webrtc (holmer) wrote:
> On 2015/09/28 11:33:20, hbos wrote:
> > On 2015/09/25 00:11:31, noahric wrote:
> > > Not sure how to set these, but there are few others I'd expect that need
to
> be
> > > set:
> > > uiIntraPeriod - keyframe period. Should be far between, if periodic
> keyframes
> > > aren't enabled.
> > > iEntropyCodingModeFlag - the recent h264 qp parser doesn't understand
cabac,
> > so
> > > make sure this is false
> > > iNumRefFrame - 1; maybe inferred from camera_video_real_time?
> > > uiProfileIdc - baseline, for fastest processing on decoders of various
> > platforms
> > > <something that disallows B frames> - maybe realtime implies this as well,
> or
> > > maybe iNumRefFrame == 1 forces it
> > 
> > Those parameters are available if SEncParamExt is used instead of
> SEncParamBase
> > and they are, amongst other parameters, set to default values with
> > GetDefaultParams. If the default values are used for the extra params not in
> > SEncParamBase I don't know whether or not there is a difference between
using
> > SEncParamBase and SEncParamExt. Switching to SEncParamExt to make more
> > parameters available but not sure if I should explicitly set all parameters
or
> > rely on GetDefaultParams.
> > 
> > 
> > Some FYI information about these parameters. They might say more to the
> > reviewers than me, much of it is like a different language to me.
> > 
> > These are comments from the source code about these properties from
> > codec_app_def.h/SEncParamExt and param_cvc.h/FillDefault.
> > - uiIntraPeriod: "period of Intra frame", "multiple of GOP size as desired"
> > - iEntropyCodingModeFlag: "0:CAVLC  1:CABAC"
> > - iNumRefFrame: "number of reference frame used"
> > - uiProfileIdc: "value of profile IDC (PRO_UNKNOWN for auto-detection)"
> > 
> > This is how the default values are set:
> >
>
https://github.com/cisco/openh264/blob/master/codec/encoder/core/inc/param_sv...
> > 
> > Here's an example of existing chromium code setting these parameters:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg...
> > 
> > iEntropyCodingModeFlag and uiProfileIdc are set appropriately by default. Do
> you
> > want me to explicitly set these anyway to make it explicit and in case the
> > openh264 default values change in the future or leave as-is after setting
> > defaults?
> > By default, iNumRefFrame = AUTO_REF_PIC_COUNT, which is = -1 with comment:
> > "encoder selects the number of reference frame automatically". Should I
really
> > set this to 1 and not use the automatic setting?
> 
> I think automatic is better. There is no reason that I know of where we don't
> want to use multiple reference frames if possible. Or are you thinking of CPU
> issues, Noah?
> 
> > As for uiIntraPeriod, I have no idea what to set this to. It defaults to 0.
> 
> 0 is no intra frame. That is good for the normal case, but if
> codec_settings->codecSpecific.H264.keyFrameInterval is set we should use that
to
> set uiIntraPeriod, I believe.
> 
> > 
> > 
> > Switched to SEncParamExt but relying on a lot of defaults atm. Had to set
> > sSpatialLayers[0] values for it to work.
> 
> Makes sense, which means you are configuring the base spatial layer (which is
> the only layer we use).
> 

Using keyFrameInterval to set uiIntraPeriod.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:218: // Failed to
initialize.
On 2015/09/28 11:19:02, stefan-webrtc (holmer) wrote:
> No need for this comment.

Done.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h (right):

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h:30: static void
RTPDefragmentize(
On 2015/09/28 11:19:02, stefan-webrtc (holmer) wrote:
> RtpDefragmentize

(Removed, no longer needed due to including start codes.)

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/test/videoprocessor.cc (right):

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor.cc:277: #if
defined(WEBRTC_OPENH264)
On 2015/09/28 11:19:02, stefan-webrtc (holmer) wrote:
> Do you need this ifdef if you have the if statement below?

(Removed, no longer needed due to including start codes.)

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/test/videoprocessor.cc:283:
H264EncoderImpl::RTPDefragmentizeBufferLengthWithNAL(encoded_image,
On 2015/09/28 11:19:02, stefan-webrtc (holmer) wrote:
> Code should probably live in this test?

(Removed, no longer needed due to including start codes.)

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (left):

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:300:
(*receive_configs)[0].rtp.nack.rtp_history_ms = kNackRtpHistoryMs;
On 2015/09/28 11:19:02, stefan-webrtc (holmer) wrote:
> I think you still want NACK to be enabled.

Done.

stefan-webrtc

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_coding/codecs/h264/h264.cc File webrtc/modules/video_coding/codecs/h264/h264.cc (right): https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_coding/codecs/h264/h264.cc#newcode55 webrtc/modules/video_coding/codecs/h264/h264.cc:55: return !IsH264CodecSupportedObjC(); On 2015/09/30 15:35:18, hbos wrote: > On ...

5 years, 2 months ago (2015-10-01 08:19:30 UTC) #20

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264.cc (right):

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264.cc:55: return
!IsH264CodecSupportedObjC();
On 2015/09/30 15:35:18, hbos wrote:
> On 2015/09/28 11:19:01, stefan-webrtc (holmer) wrote:
> > This seems a bit strange to me. Shouldn't we always first check if H264 HW
is
> > supported, and if not, check if OpenH264 is supported? We shouldn't say
> OpenH264
> > isn't supported just because we have HW. For instance, we might want to fall
> > back to OpenH264 if the HW fails.
> 
> You're right. If we are able to support both HW and SW it should be possible
to
> control and support both in code.
> 
> Right now there's just a pair of H264[En/De]coder::Create functions and no
> concept of choosing between multiple different implementations of the same
> codec. It's implementation-agnostic and I'm not sure how easy/hard it is to
fix
> this (where to decide/prioritize?).
> 
> Can I add a TODO and address this in a separate CL?

But what I'm opposed to is that IsOpenH264CodecSupported() depends on if
IsH264CodecSupportedObjC() is supported. That shouldn't matter. If
IsH264CodecSupportedObjC() returns true we should never go into this method
anyway based on H264Encoder::Create().

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264.gypi (right):

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264.gypi:29: # TODO(hbos): Add OpenH264
to third_party and update dependencies.
On 2015/09/30 15:35:18, hbos wrote:
> On 2015/09/28 11:19:01, stefan-webrtc (holmer) wrote:
> > Are we fixing this TODO before committing this CL?
> 
> Yes. I want to be able to build from source and run the trybots. But this may
be
> difficult and I wanted to kick off the review process before that to be able
to
> work in parallel.

Acknowledged.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right):

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:66:
init_params.uiCpuLoad = 0;
On 2015/09/30 15:35:18, hbos wrote:
> On 2015/09/28 11:19:02, stefan-webrtc (holmer) wrote:
> > Looks like this is unused by OpenH264, but does it make sense to set it to
0?
> > cpu load = 0 seems like bad choice.
> 
> The decoder example on github just clears all the members and then proceeds to
> set the desired members, meaning uiCpuLoad is left as 0 by the looks of it.
Not
> knowing what to set it to I did the same thing (except I listed all the
members
> and set them explicitly after the initial memset 0, for clarity). Zero may
very
> well be interpreted as unspecified.
> If not we can change this in the future if a later openh264 version gives us
> problems.

SG

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right):

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:34: return
kSkipFrame;
On 2015/09/30 15:35:18, hbos wrote:
> On 2015/09/28 11:19:02, stefan-webrtc (holmer) wrote:
> > I think we can send videoFrameTypeSkip as a kDeltaFrame. kSkipFrame has been
> > used to notify the upper layers that the frame was dropped completely by the
> > encoder, which isn't the case for an h.264 skip frame, I think?
> 
> I don't think the upper layers are notified at all unless the encode was
> successful? |encoded_image_callback_| is only used if we encoded something. I
> changed videoFrameTypeSkip to kDeltaFrame, but I'm not sure this matters or if
> it makes sense for a "skip" to be called "delta", sounds dubious.

Well, kSkipFrame and an h.264 skip-encoded frame is a bit different. A
skip-encoded frame still means that some bits are to be sent (basically telling
the decoder that the frame is identical to the previous), while a kSkipFrame
simply notifies the upper layers that the frame in question wasn't encoded.

It may not matter in practice, but I'm not sure about that.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:213:
init_params.iRCMode = RC_QUALITY_MODE;
On 2015/09/30 15:35:18, hbos wrote:
> On 2015/09/28 11:19:02, stefan-webrtc (holmer) wrote:
> > I suspect a better choice would be RC_BITRATE_MODE or RC_TIMESTAMP_MODE. I
> think
> > RC_TIMESTAMP_MODE would make most sense since then we can pass a timestamp
in
> > milliseconds with each frame and the rate controller will try to use as many
> > bits as the interval to the previous frame allows for.
> 
> I've noticed that you get really laggy video if the timestamps are not correct
> (e.g. forgetting to convert from 90kHz timestamps to ms). VideoProcessorImpl
> uses incorrect timestamp values (it uses frame count as timestamp) so at least
> one test fails if I use RC_TIMESTAMP_MODE.
> 
> Switched to RC_BITRATE_MODE and added a TODO to fix this in a later CL if we
> want to have RC_TIMESTAMP_MODE. I think RC_BITRATE_MODE is less prone to
errors
> which might be good for the initial CL.

I think RC_BITRATE_MODE makes more sense actually.

The  problem with RC_TIMESTAMP_MODE is that we will be delayed with one frame,
so we may run into problems if we have a long pause (few seconds) between frame
i-1 and frame i. The encoder may then think that it can spend a few seconds of
bit budget on frame i, which isn't true since that time has already passed... :)

https://codereview.webrtc.org/1306813009/diff/330001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right):

https://codereview.webrtc.org/1306813009/diff/330001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:105: // Fragment:
+4/-4 is for excluding the start code.
Instead of this comment you can name the constant 4. Or even better, create a
constant array:
const uint8_t kStartCode[4] = {0, 0, 0, 1};

and use it above and use sizeof(kStartCode) below.

https://codereview.webrtc.org/1306813009/diff/330001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:195: //
|keyFrameInterval| - ? number of frames? multiple of GOP size?
keyFrameInterval is in frames. I'm not sure what multiple of GOP size means when
you have an IPPP structure, where the GOP can be very large... Maybe you can try
a few settings and see how it behaves by logging when a key frame is generated?

https://codereview.webrtc.org/1306813009/diff/330001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:247:
encoded_image_buffer_.reset(nullptr);
Don't think you have to pass nullptr here.

https://codereview.webrtc.org/1306813009/diff/330001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:300: if
(std::find(frame_types->begin(), frame_types->end(),
Since we only encode a single stream you might as well DCHECK(frame_types.size()
== 1)
and then use frame_types[0].

https://codereview.webrtc.org/1306813009/diff/330001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:311: // frame it
introduces massive delays and lag in the video stream.
What the... :O

https://codereview.webrtc.org/1306813009/diff/330001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:322:
picture.uiTimeStamp = frame.timestamp() / 90;
You have to make you handle a wraparound in frame.timestamp() since it's 90 kHz
and uint32_t.

An option might be to use ntp_time_ms() instead?

hta - Chromium

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

5 years, 2 months ago (2015-10-01 09:45:50 UTC) #21

hta - Chromium

Some drive-by comments, mostly related to testing. https://codereview.webrtc.org/1306813009/diff/370001/webrtc/build/common.gypi File webrtc/build/common.gypi (right): https://codereview.webrtc.org/1306813009/diff/370001/webrtc/build/common.gypi#newcode132 webrtc/build/common.gypi:132: # CHECK ...

5 years, 2 months ago (2015-10-01 09:45:51 UTC) #22

hbos

Addressed the latest comments. https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_coding/codecs/h264/h264.cc File webrtc/modules/video_coding/codecs/h264/h264.cc (right): https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_coding/codecs/h264/h264.cc#newcode55 webrtc/modules/video_coding/codecs/h264/h264.cc:55: return !IsH264CodecSupportedObjC(); On 2015/10/01 08:19:30, ...

5 years, 2 months ago (2015-10-01 12:19:46 UTC) #23

Addressed the latest comments.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264.cc (right):

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264.cc:55: return
!IsH264CodecSupportedObjC();
On 2015/10/01 08:19:30, stefan-webrtc (holmer) wrote:
> On 2015/09/30 15:35:18, hbos wrote:
> > On 2015/09/28 11:19:01, stefan-webrtc (holmer) wrote:
> > > This seems a bit strange to me. Shouldn't we always first check if H264 HW
> is
> > > supported, and if not, check if OpenH264 is supported? We shouldn't say
> > OpenH264
> > > isn't supported just because we have HW. For instance, we might want to
fall
> > > back to OpenH264 if the HW fails.
> > 
> > You're right. If we are able to support both HW and SW it should be possible
> to
> > control and support both in code.
> > 
> > Right now there's just a pair of H264[En/De]coder::Create functions and no
> > concept of choosing between multiple different implementations of the same
> > codec. It's implementation-agnostic and I'm not sure how easy/hard it is to
> fix
> > this (where to decide/prioritize?).
> > 
> > Can I add a TODO and address this in a separate CL?
> 
> But what I'm opposed to is that IsOpenH264CodecSupported() depends on if
> IsH264CodecSupportedObjC() is supported. That shouldn't matter. If
> IsH264CodecSupportedObjC() returns true we should never go into this method
> anyway based on H264Encoder::Create().

Poor name choice. It wasn't "do we support the OpenH264 impl?" but rather "will
Create() create the OpenH264 impl?". Anyway, this function is no longer needed
so I removed it.
I also removed the TODO about controlling which impl Create() should create
because I'm not sure we need that. If we want to know about specific
implementations we can easily add IsBlablaImplSupported/CreateBlablaImpl when
that is needed.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right):

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:34: return
kSkipFrame;
On 2015/10/01 08:19:30, stefan-webrtc (holmer) wrote:
> On 2015/09/30 15:35:18, hbos wrote:
> > On 2015/09/28 11:19:02, stefan-webrtc (holmer) wrote:
> > > I think we can send videoFrameTypeSkip as a kDeltaFrame. kSkipFrame has
been
> > > used to notify the upper layers that the frame was dropped completely by
the
> > > encoder, which isn't the case for an h.264 skip frame, I think?
> > 
> > I don't think the upper layers are notified at all unless the encode was
> > successful? |encoded_image_callback_| is only used if we encoded something.
I
> > changed videoFrameTypeSkip to kDeltaFrame, but I'm not sure this matters or
if
> > it makes sense for a "skip" to be called "delta", sounds dubious.
> 
> Well, kSkipFrame and an h.264 skip-encoded frame is a bit different. A
> skip-encoded frame still means that some bits are to be sent (basically
telling
> the decoder that the frame is identical to the previous), while a kSkipFrame
> simply notifies the upper layers that the frame in question wasn't encoded.
> 
> It may not matter in practice, but I'm not sure about that.

Acknowledged.

https://codereview.webrtc.org/1306813009/diff/250001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:213:
init_params.iRCMode = RC_QUALITY_MODE;
On 2015/10/01 08:19:30, stefan-webrtc (holmer) wrote:
> On 2015/09/30 15:35:18, hbos wrote:
> > On 2015/09/28 11:19:02, stefan-webrtc (holmer) wrote:
> > > I suspect a better choice would be RC_BITRATE_MODE or RC_TIMESTAMP_MODE. I
> > think
> > > RC_TIMESTAMP_MODE would make most sense since then we can pass a timestamp
> in
> > > milliseconds with each frame and the rate controller will try to use as
many
> > > bits as the interval to the previous frame allows for.
> > 
> > I've noticed that you get really laggy video if the timestamps are not
correct
> > (e.g. forgetting to convert from 90kHz timestamps to ms). VideoProcessorImpl
> > uses incorrect timestamp values (it uses frame count as timestamp) so at
least
> > one test fails if I use RC_TIMESTAMP_MODE.
> > 
> > Switched to RC_BITRATE_MODE and added a TODO to fix this in a later CL if we
> > want to have RC_TIMESTAMP_MODE. I think RC_BITRATE_MODE is less prone to
> errors
> > which might be good for the initial CL.
> 
> I think RC_BITRATE_MODE makes more sense actually.
> 
> The  problem with RC_TIMESTAMP_MODE is that we will be delayed with one frame,
> so we may run into problems if we have a long pause (few seconds) between
frame
> i-1 and frame i. The encoder may then think that it can spend a few seconds of
> bit budget on frame i, which isn't true since that time has already passed...
:)

Acknowledged. Removed TODO comment.

https://codereview.webrtc.org/1306813009/diff/330001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right):

https://codereview.webrtc.org/1306813009/diff/330001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:105: // Fragment:
+4/-4 is for excluding the start code.
On 2015/10/01 08:19:30, stefan-webrtc (holmer) wrote:
> Instead of this comment you can name the constant 4. Or even better, create a
> constant array:
> const uint8_t kStartCode[4] = {0, 0, 0, 1};
> 
> and use it above and use sizeof(kStartCode) below.

Done.

https://codereview.webrtc.org/1306813009/diff/330001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:195: //
|keyFrameInterval| - ? number of frames? multiple of GOP size?
On 2015/10/01 08:19:30, stefan-webrtc (holmer) wrote:
> keyFrameInterval is in frames. I'm not sure what multiple of GOP size means
when
> you have an IPPP structure, where the GOP can be very large... Maybe you can
try
> a few settings and see how it behaves by logging when a key frame is
generated?

I did some logging and for me setting uiIntraPeriod to x meant every x-th frame
was a key frame (= videoFrameTypeIDR). If I made the images change drastically
(like putting my hand in front of the camera) I could get more key frames
in-between. All other frames was videoFrameTypeP.

So it works for now at least with the default video_loopback settings/frames,
but I don't know what affects the GOP size or if there is a way to check what it
is...

https://codereview.webrtc.org/1306813009/diff/330001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:247:
encoded_image_buffer_.reset(nullptr);
On 2015/10/01 08:19:30, stefan-webrtc (holmer) wrote:
> Don't think you have to pass nullptr here.

Done.

https://codereview.webrtc.org/1306813009/diff/330001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:300: if
(std::find(frame_types->begin(), frame_types->end(),
On 2015/10/01 08:19:30, stefan-webrtc (holmer) wrote:
> Since we only encode a single stream you might as well
DCHECK(frame_types.size()
> == 1)
> and then use frame_types[0].

Done. Did not realize |frame_types| one type per stream.

https://codereview.webrtc.org/1306813009/diff/330001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:311: // frame it
introduces massive delays and lag in the video stream.
On 2015/10/01 08:19:30, stefan-webrtc (holmer) wrote:
> What the... :O

Yes :) and then it crashes after a few seconds.

https://codereview.webrtc.org/1306813009/diff/330001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:322:
picture.uiTimeStamp = frame.timestamp() / 90;
On 2015/10/01 08:19:30, stefan-webrtc (holmer) wrote:
> You have to make you handle a wraparound in frame.timestamp() since it's 90
kHz
> and uint32_t.
> 
> An option might be to use ntp_time_ms() instead?

Done (using ntp_time_ms instead).

https://codereview.webrtc.org/1306813009/diff/370001/webrtc/build/common.gypi
File webrtc/build/common.gypi (right):

https://codereview.webrtc.org/1306813009/diff/370001/webrtc/build/common.gypi...
webrtc/build/common.gypi:132: # CHECK THE OPENH264 LICENCE BEFORE BUILDING, YOU
MAY LIABLE TO PAY
On 2015/10/01 09:45:51, hta - Chromium wrote:
> Grammar nit: YOU MAY LIABLE -> YOU MAY BE LIABLE
> My sense of "don't say too much" says that we should just say CHECK THE
OPENH264
> LICENSE BEFORE BUILDING.

Done.

https://codereview.webrtc.org/1306813009/diff/370001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/include/h264.h (right):

https://codereview.webrtc.org/1306813009/diff/370001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/include/h264.h:36: static bool
IsSupportedOpenH264();
On 2015/10/01 09:45:51, hta - Chromium wrote:
> I can't see anyone who calls this function.
> 1) Is it needed?
> 2) If it's needed, it should be called by an unit test. Where?
> 

My bad, it was used in a prior PS. It's not needed anymore - removing.

https://codereview.webrtc.org/1306813009/diff/370001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/1306813009/diff/370001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:285: #if defined(WEBRTC_OPENH264)
On 2015/10/01 09:45:51, hta - Chromium wrote:
> I don't like #ifdefs much - I think the style guide doesn't like them either.
> They cause compile errors to be hidden if most of the bots and so on build
> without the flag.
> 
> Do things work if you wrap the #if defined() around the RunBaseTest()
statement
> at the bottom of the test instead? That should allow the test to exist (and
> pass, I think) when WEBRTC_OPENH264 is not defined.

I don't like #ifdefs either, and it will compile if they are placed around
RunBaseTest instead. However, then all the bots that don't compile with OpenH264
will say that the SendsAndReceivesH264 test was successful, which would be very
misleading. I think that is even worse than excluding the test from non-OpenH264
builds.

stefan-webrtc

https://codereview.webrtc.org/1306813009/diff/370001/webrtc/modules/video_coding/codecs/h264/include/h264.h File webrtc/modules/video_coding/codecs/h264/include/h264.h (right): https://codereview.webrtc.org/1306813009/diff/370001/webrtc/modules/video_coding/codecs/h264/include/h264.h#newcode36 webrtc/modules/video_coding/codecs/h264/include/h264.h:36: static bool IsSupportedOpenH264(); On 2015/10/01 12:19:45, hbos wrote: > ...

5 years, 2 months ago (2015-10-01 13:13:53 UTC) #24

hbos

Description was changed from ========== H.264 video codec support using OpenH264 (http://www.openh264.org/). Implemented behind a ...

5 years, 2 months ago (2015-10-20 09:37:08 UTC) #25

palmer

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right): https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc#newcode45 webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:45: return WEBRTC_VIDEO_CODEC_ERR_PARAMETER; Ultimately it may be a good idea ...

5 years ago (2015-11-25 02:20:50 UTC) #29

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right):

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:45: return
WEBRTC_VIDEO_CODEC_ERR_PARAMETER;
Ultimately it may be a good idea to re-declare these constants as an enum, and
to return that enum type from this function/other similar functions. And then to
use switch/case in call sites.

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:48: int release_ret
= Release();
Nit: Are these codes intended to be ints, or int32s? It probably doesn't matter,
but it's better to be consistent.

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:69:
memset(&init_params, 0, sizeof(SDecodingParam));
Is this necessary? The SDecodingParam default constructor should enforce this,
right?

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:75:
init_params.sVideoProperty.eVideoBsType =
Nit: Yeah, it seems like all this could be done with a constructor call?

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:127: if
(!input_image._buffer || !input_image._length)
BUG?: It looks like _length is declared as a size_t, but you pass it to
DecodeFrameNoDelay which treats it as a signed int. This is a change in
signedness and possibly in width, so it's a potentially unsafe cast.

I haven't yet seen the implementation of DecodeFrameNoDelay, so I don't yet know
what it's going to do with its arguments, but it seems like retaining type
information and checking for more than just !input_image._length would be a very
good idea. Deserializing data from the internet is always dangerous...

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:137:
memset(&buffer_info, 0, sizeof(SBufferInfo));
Same default constructor issue here, too. (Unless I'm misunderstanding something
and there is a reason you have to use this old style?)

palmer

I strongly recommend setting bounds on the sizes and offsets, and/or using a safe integer ...

5 years ago (2015-11-26 01:30:37 UTC) #30

hbos

PTAL palmer. Not using Chromium's base/numeric because we're in the WebRTC repo, but there's rtc::IsValueInRangeForNumericType. ...

5 years ago (2015-11-27 14:43:43 UTC) #32

PTAL palmer.

Not using Chromium's base/numeric because we're in the WebRTC repo, but there's
rtc::IsValueInRangeForNumericType.

https://codereview.webrtc.org/1306813009/diff/370001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/include/h264.h (right):

https://codereview.webrtc.org/1306813009/diff/370001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/include/h264.h:36: static bool
IsSupportedOpenH264();
On 2015/10/01 13:13:52, stefan-webrtc (holmer) wrote:
> On 2015/10/01 12:19:45, hbos wrote:
> > On 2015/10/01 09:45:51, hta - Chromium wrote:
> > > I can't see anyone who calls this function.
> > > 1) Is it needed?
> > > 2) If it's needed, it should be called by an unit test. Where?
> > > 
> > 
> > My bad, it was used in a prior PS. It's not needed anymore - removing.
> 
> Then I guess you want to delete it? :)

Oh, alright then! :)

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right):

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:45: return
WEBRTC_VIDEO_CODEC_ERR_PARAMETER;
On 2015/11/25 02:20:50, palmer wrote:
> Ultimately it may be a good idea to re-declare these constants as an enum, and
> to return that enum type from this function/other similar functions. And then
to
> use switch/case in call sites.

Acknowledged. (Won't do that as part of this work though, it's used in a million
places.)

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:48: int release_ret
= Release();
On 2015/11/25 02:20:50, palmer wrote:
> Nit: Are these codes intended to be ints, or int32s? It probably doesn't
matter,
> but it's better to be consistent.

Done.

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:69:
memset(&init_params, 0, sizeof(SDecodingParam));
On 2015/11/25 02:20:50, palmer wrote:
> Is this necessary? The SDecodingParam default constructor should enforce this,
> right?

It's necessary. It's a struct with a default constructor that doesn't initialize
its primitive variables and memset is what the OpenH264 tutorial code does :)
(Open source third party code maintained by Cisco, they do things differently
than we'd like)

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:75:
init_params.sVideoProperty.eVideoBsType =
On 2015/11/25 02:20:50, palmer wrote:
> Nit: Yeah, it seems like all this could be done with a constructor call?

I'm afraid not (unless we start contributing to the OpenH264 project and add a
sensible constructor)

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:127: if
(!input_image._buffer || !input_image._length)
On 2015/11/25 02:20:50, palmer wrote:
> BUG?: It looks like _length is declared as a size_t, but you pass it to
> DecodeFrameNoDelay which treats it as a signed int. This is a change in
> signedness and possibly in width, so it's a potentially unsafe cast.
> 
> I haven't yet seen the implementation of DecodeFrameNoDelay, so I don't yet
know
> what it's going to do with its arguments, but it seems like retaining type
> information and checking for more than just !input_image._length would be a
very
> good idea. Deserializing data from the internet is always dangerous...

Hmh I don't know if it's possible to receive crazy big frames but I added a
check to ensure it can be converted safely to int.

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:137:
memset(&buffer_info, 0, sizeof(SBufferInfo));
On 2015/11/25 02:20:50, palmer wrote:
> Same default constructor issue here, too. (Unless I'm misunderstanding
something
> and there is a reason you have to use this old style?)

See other comment.

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:173:
decoded_image_.set_ntp_time_ms(input_image.ntp_time_ms_);
On 2015/10/01 13:13:52, stefan-webrtc (holmer) wrote:
> I think you have to call set_render_time_ms() too.

Done. Looks like every encoder uses a different timestamp :P

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.h (right):

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.h:43: bool
IsInitialized();
On 2015/10/01 13:13:52, stefan-webrtc (holmer) wrote:
> Private?

Ok sure.

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right):

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:47: // Helper
method used by H264EncoderImpl::Encode.
On 2015/11/26 01:30:37, palmer wrote:
> Nit: Add a blank line.

Done.

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:51: //   After
OpenH264 encoding, the encoded bytes are stored in |info| spread out
On 2015/11/26 01:30:37, palmer wrote:
> Nit: Blank line to start a new paragraph instead of indent.

Done.

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:65: for (int iLayer
= 0; iLayer < info->iLayerNum; ++iLayer) {
On 2015/11/26 01:30:37, palmer wrote:
> What is the type of |info->iLayerNum|? 

It's also int so that's fine.

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:68: required_size
+= layerInfo.pNalLengthInByte[iNal];
On 2015/11/26 01:30:37, palmer wrote:
> Is the value |layerInfo.pNalLengthInByte[iNal]| trustworthy? What stops
> |required_size| from overflowing?

I expect it's "trustworthy" unless there is a bug. I added an RTC_CHECKs to
ensure that it's not negative and can't overflow.

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:72: if
(encoded_image->_size < required_size) {
On 2015/11/26 01:30:37, palmer wrote:
> If |required_size| overflowed, this condition may evaluate to false, but that
> could be incorrect.

Now it won't overflow and we've validated pNalLengthInByte in calculating
|required_size|.

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:96: for (int iLayer
= 0; iLayer < info->iLayerNum; ++iLayer) {
On 2015/10/01 13:13:52, stefan-webrtc (holmer) wrote:
> I'd prefer if you rename all the camelCase variables here and below. And
remove
> "i" in front of them.

Done.

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:106:
encoded_image->_length + iLayerLen + sizeof(kStartCode);
On 2015/11/26 01:30:37, palmer wrote:
> I suspect this expression could overflow, and lines 107 – 109 as well.

Now that we've ensured |required_size| won't overflow, which is the sum of all
the bytes that we're copying (pNalLengthInBytes), overflow in this code should
be impossible.

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:114: iLayerLen *
sizeof(unsigned char));
On 2015/11/26 01:30:37, palmer wrote:
> Note that sizeof(/* anything */ char) is, by definition, 1.
> 
> Shouldn't |iLayerLen| be the same as |required_len|, at this point?

Not if there are multiple layers. Removed sizeof(unsigned char).

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:115:
encoded_image->_length += iLayerLen;
On 2015/11/26 01:30:37, palmer wrote:
> This could overflow.

Not anymore, see other comment.

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:328: memset(&info,
0, sizeof(SFrameBSInfo));
On 2015/11/26 01:30:37, palmer wrote:
> Same default constructor questions as before (and presumably throughout).

See other comment.

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h (right):

https://codereview.webrtc.org/1306813009/diff/390001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h:52: bool
IsInitialized();
On 2015/10/01 13:13:52, stefan-webrtc (holmer) wrote:
> Can this be private?

Yes! :)))

hbos

Description was changed from ========== H.264 video codec support using OpenH264 (http://www.openh264.org/). Implemented behind a ...

5 years ago (2015-12-16 18:13:17 UTC) #34

kjellander_webrtc

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

4 years, 11 months ago (2016-01-04 14:52:07 UTC) #36

kjellander_webrtc

https://codereview.webrtc.org/1306813009/diff/570001/chromium/.gclient File chromium/.gclient (left): https://codereview.webrtc.org/1306813009/diff/570001/chromium/.gclient#oldcode13 chromium/.gclient:13: 'src/third_party/ffmpeg': None, (as discussed offline) just changing this file ...

4 years, 11 months ago (2016-01-04 14:52:08 UTC) #37

hbos

Please take a look, reviewers. I want to land this (behind a flag). Now there ...

4 years, 11 months ago (2016-01-04 15:49:06 UTC) #38

noahric

Some comments on the ffmpeg decoder bits, but mostly just stylistic nits. Overall LGTM. https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_coding/codecs/h264/h264.cc ...

4 years, 11 months ago (2016-01-04 20:49:30 UTC) #39

Some comments on the ffmpeg decoder bits, but mostly just stylistic nits.
Overall LGTM.

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264.cc (right):

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264.cc:70: return new
H264VideoToolboxDecoder();
If you can log here, consider adding log statements for which encoder/decoder
implementations are used. It may be surprising in some circumstance to end up
with a software encoder/decoder (i.e. if mac enables video toolbox support and
for some reason doesn't have it compiled in).

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right):

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:76: // reference
counted and freed by FFmpeg using AVFreeBuffer2.
Is it also true that the AVFrame is basically unused? There are a lot of things
set on it, but in Decode, it's ignored in favor of the opaque void* with the
associated VideoFrame. Does that mean you could get away with not setting many
of the values and basically just setting the buf[0] field?

Either way, it may be helpful to comment that the *real* video frame will be
stored in an associated VideoFrame and not in the AVFrame itself.

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:79: static int
AVGetBuffer2(AVCodecContext* context, AVFrame* frame, int flags) {
consider naming this out_frame or av_frame. Normally I don't put types into
variables, but in this case, it may help distinguish it below from video_frame.

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:111: // individual
plane.
Sounds like an issue for i420 frame, then. Is it worth creating an issue for
this and not just a TODO (since the fix will be pretty far away from this
implementation)?

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:115: // Expect YUV
to be a continuous blob of memory so that we can zero-initialize
Is it worth it to just do three memsets? It's probably about as fast, and
doesn't rely on the assumption the planes are contiguous.

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:128:
frame->data[kYPlane] = video_frame->buffer(kYPlane);
If the absolute values of these matter (it looks like it doesn't, since they are
unused, other than DCHECKs), then consider using a second set of constants in
this file, in the random chance that kYPlane will change away from 0 at some
point :) Or hardcode 0,1,2, like you did in the encoder.

Or if it doesn't matter, then just ignore this comment.

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:136: frame->buf[0]
= av_buffer_create(frame->data[0],
It looks like this is also requiring that the data be contiguous, so it's not
just that you want it that way to be zero-initialized. Can you add this to the
comment above (around line 115) and change this to kYPlane?

(If it isn't really a requirement, I'd at least change 0 to kYPlane)

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:164: #if
!defined(WEBRTC_CHROMIUM_BUILD)
Consider putting this behind a different flag, if it's possible that some other
build will also initialize ffmpeg outside of webrtc.

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:181: // baseline
profile.
I assume it's also meant to be able to decode streams generated by any standard
webrtc client, which could be non-baseline. Though I'd be surprised if this
actually did anything, since I assume the decoder will determine the profile by
looking at the bitstream.

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:191:
av_context_->thread_count = 4;
Should this be gleaned from something else? Number of cores, maybe? Encoders
have something similar, maybe you could find/copy that logic?

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:266: // only be a
problem with non-mul-32 frame widths?
I'd buy the second part of this sentence, and it's probably an easy-enough
change to make VCMEncodedFrame::VerifyAndAllocate produce frames that round up
to the nearest 64 bytes (and then update Decode documentation to say that
implementations can rely on the extra bytes). Worth opening a bug for this as
well?

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:268: // bitstreams
could cause overread and segfault.")
I think this could happen anyways today (VCMFrameBuffer over-allocates when
receiving packets but doesn't obviously zero out that memory). Can you TODO or
open a bug for this? Especially given that scary warning about segfaults.

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:271:
av_context_->reordered_opaque = input_image.ntp_time_ms_ * 1000;  // ms -> μs
Can you add a comment here? avcodec.h says this is deprecated in favor of
pkt_pts, unless that's a lie :)

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:318: return
av_context_;
av_context_ != nullptr

(implicit cast to bool will produce warnings on some platforms, I believe)

hbos

Description was changed from ========== H.264 video codec support using OpenH264 (http://www.openh264.org/) for encoding and ...

4 years, 11 months ago (2016-01-07 11:41:14 UTC) #40

hbos

PTAL stefan, mflodman, hta. Hoping this can land real soon. If so we might be ...

4 years, 11 months ago (2016-01-07 19:41:15 UTC) #41

PTAL stefan, mflodman, hta.

Hoping this can land real soon. If so we might be able to support H264 in Chrome
behind a flag for the M49 cut next Friday. Just maybe...

(Note that I plan to address the TODO-comments in h264_decoder_impl.cc in
follow-up CLs)

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264.cc (right):

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264.cc:70: return new
H264VideoToolboxDecoder();
On 2016/01/04 20:49:30, noahric wrote:
> If you can log here, consider adding log statements for which encoder/decoder
> implementations are used. It may be surprising in some circumstance to end up
> with a software encoder/decoder (i.e. if mac enables video toolbox support and
> for some reason doesn't have it compiled in).

Done.

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right):

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:76: // reference
counted and freed by FFmpeg using AVFreeBuffer2.
On 2016/01/04 20:49:30, noahric wrote:
> Is it also true that the AVFrame is basically unused? There are a lot of
things
> set on it, but in Decode, it's ignored in favor of the opaque void* with the
> associated VideoFrame. Does that mean you could get away with not setting many
> of the values and basically just setting the buf[0] field?
> 
> Either way, it may be helpful to comment that the *real* video frame will be
> stored in an associated VideoFrame and not in the AVFrame itself.

It is used by ffmpeg, we are required to set certain members such as data,
linesize and buf in the callback. I don't know if format and reordered_opaque is
used by ffmpeg though, which I also set.
There's no need to set width and height though, these are already set for us by
FFmpeg.

Added a comment and width/height DCHECKs.

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:79: static int
AVGetBuffer2(AVCodecContext* context, AVFrame* frame, int flags) {
On 2016/01/04 20:49:30, noahric wrote:
> consider naming this out_frame or av_frame. Normally I don't put types into
> variables, but in this case, it may help distinguish it below from
video_frame.

Done.

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:111: // individual
plane.
On 2016/01/04 20:49:30, noahric wrote:
> Sounds like an issue for i420 frame, then. Is it worth creating an issue for
> this and not just a TODO (since the fix will be pretty far away from this
> implementation)?

I read some FFmpeg documentation a second time and I'm confused if "data plane"
in this context refers to a YUV-plane (which is what I originally thought) or a
buf[] buffer. For example:

"There may be one contiguous buffer for all the data or there may be a buffer
per each data plane or anything in between. What this means is, you may set
however many entries in buf[] you feel necessary. [...] Each data plane must be
aligned to the maximum required by the target CPU."

If only each buf[] entry needs to be aligned then there is nothing we have
TO-DO. Otherwise it makes sense to create a separate issue.
Considering buf[0] = av_buffer_create(...) also requires it to be continuous it
makes little sense if each YUV-plane inside of it has to have memory alignment
that might not match up with the YUV-planes based on frame dimensions.

Considering removing the TODO instead, but have not done so yet. Hesitating.

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:115: // Expect YUV
to be a continuous blob of memory so that we can zero-initialize
On 2016/01/04 20:49:30, noahric wrote:
> Is it worth it to just do three memsets? It's probably about as fast, and
> doesn't rely on the assumption the planes are contiguous.

Probably not. But as you pointed out, it looks like it is required to be
continuous anyway.

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:128:
frame->data[kYPlane] = video_frame->buffer(kYPlane);
On 2016/01/04 20:49:30, noahric wrote:
> If the absolute values of these matter (it looks like it doesn't, since they
are
> unused, other than DCHECKs), then consider using a second set of constants in
> this file, in the random chance that kYPlane will change away from 0 at some
> point :) Or hardcode 0,1,2, like you did in the encoder.
> 
> Or if it doesn't matter, then just ignore this comment.

Done.

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:136: frame->buf[0]
= av_buffer_create(frame->data[0],
On 2016/01/04 20:49:30, noahric wrote:
> It looks like this is also requiring that the data be contiguous, so it's not
> just that you want it that way to be zero-initialized. Can you add this to the
> comment above (around line 115) and change this to kYPlane?
> 
> (If it isn't really a requirement, I'd at least change 0 to kYPlane)

Done.

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:164: #if
!defined(WEBRTC_CHROMIUM_BUILD)
On 2016/01/04 20:49:30, noahric wrote:
> Consider putting this behind a different flag, if it's possible that some
other
> build will also initialize ffmpeg outside of webrtc.

Good point. Added a TODO, I'll address this in a follow-up CL at the same time
as I address my other init-related TODO.

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:181: // baseline
profile.
On 2016/01/04 20:49:30, noahric wrote:
> I assume it's also meant to be able to decode streams generated by any
standard
> webrtc client, which could be non-baseline. Though I'd be surprised if this
> actually did anything, since I assume the decoder will determine the profile
by
> looking at the bitstream.

You're right! I looked this up, it should not be set by us, removing line.

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:191:
av_context_->thread_count = 4;
On 2016/01/04 20:49:30, noahric wrote:
> Should this be gleaned from something else? Number of cores, maybe? Encoders
> have something similar, maybe you could find/copy that logic?

Done. Copied from vp8_impl.cc.

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:266: // only be a
problem with non-mul-32 frame widths?
On 2016/01/04 20:49:30, noahric wrote:
> I'd buy the second part of this sentence, and it's probably an easy-enough
> change to make VCMEncodedFrame::VerifyAndAllocate produce frames that round up
> to the nearest 64 bytes (and then update Decode documentation to say that
> implementations can rely on the extra bytes). Worth opening a bug for this as
> well?

(I'll address this tomorrow, getting late)

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:271:
av_context_->reordered_opaque = input_image.ntp_time_ms_ * 1000;  // ms -> μs
On 2016/01/04 20:49:30, noahric wrote:
> Can you add a comment here? avcodec.h says this is deprecated in favor of
> pkt_pts, unless that's a lie :)

Lies! In some versions of ffmpeg it is deprecated, but the third_party/ffmpeg
version of avcodec.h is not deprecated. (I apparently have two versions of the
header on my machine and the other has it deprecated.)

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:318: return
av_context_;
On 2016/01/04 20:49:30, noahric wrote:
> av_context_ != nullptr
> 
> (implicit cast to bool will produce warnings on some platforms, I believe)

Done.

https://codereview.webrtc.org/1306813009/diff/630001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h (right):

https://codereview.webrtc.org/1306813009/diff/630001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.h:64:
rtc::scoped_ptr<uint8_t[]> encoded_image_buffer_;
(Ensures new[] is matched with delete[] and not delete. Required when running a
Chromium build.)

kjellander_webrtc

lgtm on the infrastructure changes (*.gn*,*.gyp*,*.py etc) https://codereview.webrtc.org/1306813009/diff/630001/webrtc/modules/video_coding/codecs/h264/h264.gypi File webrtc/modules/video_coding/codecs/h264/h264.gypi (right): https://codereview.webrtc.org/1306813009/diff/630001/webrtc/modules/video_coding/codecs/h264/h264.gypi#newcode31 webrtc/modules/video_coding/codecs/h264/h264.gypi:31: '<(DEPTH)/third_party/openh264/openh264.gyp:openh264_encoder', Sort ...

4 years, 11 months ago (2016-01-08 07:56:24 UTC) #42

stefan-webrtc

lgtm https://codereview.webrtc.org/1306813009/diff/630001/webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right): https://codereview.webrtc.org/1306813009/diff/630001/webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc#newcode81 webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:81: return 3; Comment on the same line as ...

4 years, 11 months ago (2016-01-08 09:25:05 UTC) #43

hbos

@palmer: After the LGTM to proceed with OpenH264 for encoding and FFmpeg for decoding I ...

4 years, 11 months ago (2016-01-11 16:21:53 UTC) #44

@palmer: After the LGTM to proceed with OpenH264 for encoding and FFmpeg for
decoding I wasn't sure if you wanted to continue to review this or not from a
security perspective. If so, please take a look at enc/dec :)

PTAL mflodman.

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right):

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:266: // only be a
problem with non-mul-32 frame widths?
On 2016/01/07 19:41:14, hbos wrote:
> On 2016/01/04 20:49:30, noahric wrote:
> > I'd buy the second part of this sentence, and it's probably an easy-enough
> > change to make VCMEncodedFrame::VerifyAndAllocate produce frames that round
up
> > to the nearest 64 bytes (and then update Decode documentation to say that
> > implementations can rely on the extra bytes). Worth opening a bug for this
as
> > well?
> 
> (I'll address this tomorrow, getting late)

Thanks for the info, yeah should be easy. Created a bug.

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:268: // bitstreams
could cause overread and segfault.")
On 2016/01/04 20:49:30, noahric wrote:
> I think this could happen anyways today (VCMFrameBuffer over-allocates when
> receiving packets but doesn't obviously zero out that memory). Can you TODO or
> open a bug for this? Especially given that scary warning about segfaults.

Done. (Same bug)

https://codereview.webrtc.org/1306813009/diff/630001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264.gypi (right):

https://codereview.webrtc.org/1306813009/diff/630001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264.gypi:31:
'<(DEPTH)/third_party/openh264/openh264.gyp:openh264_encoder',
On 2016/01/08 07:56:24, kjellander (webrtc) wrote:
> Sort alphabetically

Done.

https://codereview.webrtc.org/1306813009/diff/630001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right):

https://codereview.webrtc.org/1306813009/diff/630001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:81: return 3;
On 2016/01/08 09:25:04, stefan-webrtc (holmer) wrote:
> Comment on the same line as above? Same below.

Done.

palmer

https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right): https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc#newcode39 webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:39: // Called by FFmpeg to do mutex operations if ...

4 years, 11 months ago (2016-01-11 22:27:09 UTC) #45

mflodman

https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right): https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc#newcode76 webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:76: static int NumberOfThreads(int width, int height, int number_of_cores) { ...

4 years, 11 months ago (2016-01-12 10:31:25 UTC) #46

hta-webrtc

hta@webrtc.org changed reviewers: - hta@chromium.org

4 years, 11 months ago (2016-01-12 10:53:33 UTC) #47

hta-webrtc

Metacommentary... https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right): https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc#newcode84 webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:84: // Encoded data > unencoded data, wtf? Allocate ...

4 years, 11 months ago (2016-01-12 10:53:34 UTC) #48

stefan-webrtc

https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right): https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc#newcode84 webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:84: // Encoded data > unencoded data, wtf? Allocate required ...

4 years, 11 months ago (2016-01-12 11:18:33 UTC) #49

mflodman

https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right): https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc#newcode84 webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:84: // Encoded data > unencoded data, wtf? Allocate required ...

4 years, 11 months ago (2016-01-12 11:25:34 UTC) #50

https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right):

https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:84: // Encoded data
> unencoded data, wtf? Allocate required bytes.
On 2016/01/12 11:18:33, stefan-webrtc (holmer) wrote:
> On 2016/01/12 10:53:34, hta-webrtc wrote:
> > On 2016/01/12 10:31:25, mflodman wrote:
> > > Seems like this really shouldn't happen, is it worth having this logic and
> > will
> > > we in that case ever catch this log message? Should we have some kind of
> check
> > > instead?
> > 
> > Video encoding is complex enough that we can't guarantee it won't.
> > I suspect that the only way to get here is specially crafted images that are
> > designed to be uncompressible (possibly part of an attack), but there's no
way
> > to guarantee it, and no way to guarantee it won't happen by accident either.
> > 
> > For instance, white noise images are, by definition almost, uncompressible.
> 
> Agree, even though very unlikely, it's better not to crash if this happens.

Fair enough, but then I'd prefer to remove the 'wtf' and maybe even the log if
it's not useful for anything. WDYT?

https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:204:
init_params.iMultipleThreadIdc = 0;
On 2016/01/12 10:53:34, hta-webrtc wrote:
> On 2016/01/12 10:31:25, mflodman wrote:
> > Do we know what this leads to in the Chrome sandbox, where I assume (maybe
> > incorrectly) the encoder can't read the number of cores?
> 
> I think Tommi added the ability to read number of cores from within the
sandbox
> recently.

I know we do get the correct number through the input parameter to this method,
but I didn't know it was solved for the generic case. Great if that is the case,
but if it isn't I'd prefer to use 'number_of_cores' and explicitly set the
number of threads.

Great if that is the case, if not I think it's better to explicitly set the
number of threads.

hbos

Addressed comments. PTAL palmer, mflodman. https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right): https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc#newcode111 webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:111: // individual plane. On ...

4 years, 11 months ago (2016-01-12 13:56:27 UTC) #51

Addressed comments. PTAL palmer, mflodman.

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right):

https://codereview.webrtc.org/1306813009/diff/610001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:111: // individual
plane.
On 2016/01/07 19:41:14, hbos wrote:
> On 2016/01/04 20:49:30, noahric wrote:
> > Sounds like an issue for i420 frame, then. Is it worth creating an issue for
> > this and not just a TODO (since the fix will be pretty far away from this
> > implementation)?
> 
> I read some FFmpeg documentation a second time and I'm confused if "data
plane"
> in this context refers to a YUV-plane (which is what I originally thought) or
a
> buf[] buffer. For example:
> 
> "There may be one contiguous buffer for all the data or there may be a buffer
> per each data plane or anything in between. What this means is, you may set
> however many entries in buf[] you feel necessary. [...] Each data plane must
be
> aligned to the maximum required by the target CPU."
> 
> If only each buf[] entry needs to be aligned then there is nothing we have
> TO-DO. Otherwise it makes sense to create a separate issue.
> Considering buf[0] = av_buffer_create(...) also requires it to be continuous
it
> makes little sense if each YUV-plane inside of it has to have memory alignment
> that might not match up with the YUV-planes based on frame dimensions.
> 
> Considering removing the TODO instead, but have not done so yet. Hesitating.

Removing the TODO based on new interpretation of the comment, on the requirement
that it be continuous, and by looking at chromium's allocation.

https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right):

https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:39: // Called by
FFmpeg to do mutex operations if init using InitializeFFmpeg.
On 2016/01/11 22:27:09, palmer wrote:
> Nit: use |...| to signal identifiers. For example, I don't know if "init" and
> "IntializeFFmpeg" are identifiers in this comment.

Done.

https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:40: static int
LockManagerOperation(void** lock, AVLockOp op)
On 2016/01/11 22:27:09, palmer wrote:
> You don't need to declare things in the anonymous namespace as static — they
are
> private to this compilation unit by virtue of being in the anonymous
namespace.

Done.

I think anonymous namespace is preferred to the static keyword, but I typically
still use the keyword to make it extra clear that it's private in case one
misses the fact that we're in that namespace on a particular line of code. But
sure, it's superfluous, I'll remove the unnecessary statics.

https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:57: return 1;
On 2016/01/11 22:27:09, palmer wrote:
> Since the return values are just 0 and 1, should they be true and false
instead,
> and the return type declared as bool?
> 
> Shouldn't you use NOTREACHED here, too?

Return type: FFmpeg defines it to return int. C style. The Dude abides.

NOTREACHED: You're right. Returning non-zero means the operation failed, but if
it fails due to us not recognizing an AVLockOp we must update our implementation
and crashing is the way to go to make this evident.

https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:60: // TODO(hbos):
Assumed to be called on a single thread. Should DCHECK that
On 2016/01/11 22:27:09, palmer wrote:
> Link to a bug in all TODOs.

Done.

https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:65: LOG(LS_ERROR)
<< "av_lockmgr_register failed.";
On 2016/01/11 22:27:09, palmer wrote:
> Should this ever happen? Should it be CHECK or NOTREACHED?

Done.

https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:76: static int
NumberOfThreads(int width, int height, int number_of_cores) {
On 2016/01/12 10:31:25, mflodman wrote:
> See my comment below about threads.

Acknowledged.

https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:90: VideoFrame*
video_frame = static_cast<VideoFrame*>(opaque);
On 2016/01/11 22:27:09, palmer wrote:
> This looks dangerous. What guarantee is there that |opaque| is safely castable
> to |VideoFrame*|? (Or even that it was allocated on the free store?) There
> should be some type-safe way to release these objects — scoped_ptr,
unique_ptr,
> et c. See also |DeleteSoon| in Chromium (base/sequenced_task_runner.h).

The only "guarantee" is that for each VideoFrame we allocate we tell FFmpeg to
call |AVFreeBuffer2| when that very buffer should be freed, |AVFreeBuffer2| is
not used for anything else. It's guaranteed by the API, but technically it is
not type-safe no.

In |AVGetBuffer2| we set a void*, in |AVFreeBuffer2| we get that void* and are
responsible for for deleting it. If there is a bug that makes this function be
called with something else we wouldn't know (without RTTI?) unless we keep a
record of all the buffers and verify that it is indeed one of the buffers before
we delete it.
- If you want, I could make this a feature of the frame buffer pool (pool = TODO
for a follow-up)? Then the void* works more like an identifier for the buffer
and we don't necessarily have to cast. Otherwise, I think it's safe to cast
based on the API.

https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:124: size_t
total_size = video_frame->allocated_size(kYPlane) +
On 2016/01/11 22:27:09, palmer wrote:
> Could this arithmetic overflow?

No, av_image_check_size (called above) makes sure the dimensions are safe:
"Check if the given dimension of an image is valid, meaning that all bytes of
the image can be addressed with a signed int."

Thus, calculating the size will not overflow. (Technically, the size = one past
the last byte, could be an overflow, but you can't achieve a resolution like
this with (int width) by (int height) when both w and h are >= 0.)

https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:127:
RTC_DCHECK_EQ(total_size, static_cast<size_t>(stride_y * height +
On 2016/01/11 22:27:09, palmer wrote:
> And this arithmetic.
> 
> Also, casting the result to size is not necessarily the same as correctly
> casting each operand to size_t before doing the arithmetic.

See previous comment. Switched to int instead of size_t to not have to cast.

https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:214:
av_context_->thread_count = NumberOfThreads(av_context_->coded_width,
On 2016/01/12 10:31:24, mflodman wrote:
> This seems like a lot of threads for decoding, for VP8 and VP9 we always use
> only one for decode but multiple for encode. I'd like to set only one here too
> unless we've seen good gains by this, but in that case use less than is done
> here.

OK. 1 it is.

https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:289: // - Is this
an issue? Do we have to make sure EncodedImage is allocated with
On 2016/01/11 22:27:09, palmer wrote:
> Yes, this is an issue. Round the allocation size up.

Will address in follow-up CL. There is a bug created for this. Will not unflag
H264 until all TODOs are addressed.

https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.h (right):

https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.h:38: //
|codec_settings->codecType| must be kVideoCodecH264.
On 2016/01/11 22:27:09, palmer wrote:
> This comment reads a bit weirdly; sort of like a double-negative. How about:
> 
> If |codec_settings| is NULL, it is ignored. If |codec_settings| is not NULL,
> |codec_settings->codecType| must be |kVideoCodecH264|.

Done.

https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right):

https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:84: // Encoded data
> unencoded data, wtf? Allocate required bytes.
On 2016/01/12 11:25:34, mflodman wrote:
> On 2016/01/12 11:18:33, stefan-webrtc (holmer) wrote:
> > On 2016/01/12 10:53:34, hta-webrtc wrote:
> > > On 2016/01/12 10:31:25, mflodman wrote:
> > > > Seems like this really shouldn't happen, is it worth having this logic
and
> > > will
> > > > we in that case ever catch this log message? Should we have some kind of
> > check
> > > > instead?
> > > 
> > > Video encoding is complex enough that we can't guarantee it won't.
> > > I suspect that the only way to get here is specially crafted images that
are
> > > designed to be uncompressible (possibly part of an attack), but there's no
> way
> > > to guarantee it, and no way to guarantee it won't happen by accident
either.
> > > 
> > > For instance, white noise images are, by definition almost,
uncompressible.
> > 
> > Agree, even though very unlikely, it's better not to crash if this happens.
> 
> Fair enough, but then I'd prefer to remove the 'wtf' and maybe even the log if
> it's not useful for anything. WDYT?

I was thinking the same. (Imagine if screensharing random pixels made the other
end hit a CHECK, nice way to troll a public hangout?) Removed the "wtf" :)

With the logging I'm thinking its unlikely enough that it wouldn't happen under
normal circumstances and could be interesting to know if it happened. But not
feeling strongly about this, if you want me to remove the log I will.

https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:204:
init_params.iMultipleThreadIdc = 0;
On 2016/01/12 11:25:34, mflodman wrote:
> On 2016/01/12 10:53:34, hta-webrtc wrote:
> > On 2016/01/12 10:31:25, mflodman wrote:
> > > Do we know what this leads to in the Chrome sandbox, where I assume (maybe
> > > incorrectly) the encoder can't read the number of cores?
> > 
> > I think Tommi added the ability to read number of cores from within the
> sandbox
> > recently.
> 
> I know we do get the correct number through the input parameter to this
method,
> but I didn't know it was solved for the generic case. Great if that is the
case,
> but if it isn't I'd prefer to use 'number_of_cores' and explicitly set the
> number of threads.
> 
> Great if that is the case, if not I think it's better to explicitly set the
> number of threads.

Ok. I'll use the NumberOfThreads function copied from vp9_impl.cc (which I
previously used on the decode side but removed).

mflodman

LGTM https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right): https://codereview.webrtc.org/1306813009/diff/650001/webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc#newcode84 webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:84: // Encoded data > unencoded data, wtf? Allocate ...

4 years, 11 months ago (2016-01-12 13:58:13 UTC) #52

hbos

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

4 years, 11 months ago (2016-01-12 14:45:31 UTC) #53

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1306813009/690001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1306813009/690001

4 years, 11 months ago (2016-01-12 14:45:48 UTC) #54

commit-bot: I haz the power

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

4 years, 11 months ago (2016-01-12 14:49:45 UTC) #55

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios32_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/4431) ios_arm64_rel on ...

4 years, 11 months ago (2016-01-12 14:49:46 UTC) #56

hbos

On 2016/01/12 14:49:46, commit-bot: I haz the power wrote: > Dry run: Try jobs failed ...

4 years, 11 months ago (2016-01-12 15:34:14 UTC) #57

hbos

On 2016/01/12 15:34:14, hbos wrote: > On 2016/01/12 14:49:46, commit-bot: I haz the power wrote: ...

4 years, 11 months ago (2016-01-12 15:45:18 UTC) #58

palmer

palmer@chromium.org changed reviewers: + mbarbella@chromium.org

4 years, 11 months ago (2016-01-20 19:17:46 UTC) #59

palmer

I really think you should make use of a safe arithmetic library. I know that ...

4 years, 11 months ago (2016-01-20 19:17:47 UTC) #60

I really think you should make use of a safe arithmetic library. I know that
WebRTC doesn't have access to Chromium's base/, which is why we simply copied
base/numerics into PDFium. I think that'd be a good idea here, too. When
manually parsing these binary formats using offsets and counts that come from
the untrustworthy network, out-of-bounds reads and writes are a classic and
common bug.

Speaking of which, are there fuzzers for this code in ClusterFuzz? +mbarbella

https://codereview.chromium.org/1306813009/diff/710001/webrtc/modules/video_c...
File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right):

https://codereview.chromium.org/1306813009/diff/710001/webrtc/modules/video_c...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:90: // cast. See
https://bugs.chromium.org/p/webrtc/issues/detail?id=5428.
Cool, thanks!

Nit: delineate identifiers with |...|.

https://codereview.chromium.org/1306813009/diff/710001/webrtc/modules/video_c...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:105: if (ret < 0) {
Looking at the implementation of |av_image_check_size|, 0 is the sole
"everything is OK" return value. So you can make this check tighter by doing:

  if (ret) { ... }

or, if you like,

  if (0 != ret) { ... }

https://codereview.chromium.org/1306813009/diff/710001/webrtc/modules/video_c...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right):

https://codereview.chromium.org/1306813009/diff/710001/webrtc/modules/video_c...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:118: for (int nal =
0; nal < layerInfo.iNalCount; ++nal, ++frag) {
If I am reading this code right, and I am not sure I am, |fragments_count| is
guaranteed to equal info->iLayerNum, due to the loop in lines 79 – 90. Is that
right?

But here, you increment |frag| layerInfo.iNalCount times. Is layerInfo.iNalCount
guaranteed to equal info->iLayerNum? Are there guaranteed to be
layerInfo.iNalCount fragments in frag_header->fragmentationOffset?

https://codereview.chromium.org/1306813009/diff/710001/webrtc/modules/video_c...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:123:
frag_header->fragmentationOffset[frag] =
Can you add a comment explaining why the arithmetic in lines 123 – 127 is
guaranteed not to overflow or underflow?

Martin Barbella

On 2016/01/20 19:17:47, palmer wrote: > I really think you should make use of a ...

4 years, 11 months ago (2016-01-20 20:33:58 UTC) #61

hbos_chromium

Description was changed from ========== H.264 video codec support using OpenH264 (http://www.openh264.org/) for encoding and ...

4 years, 11 months ago (2016-01-21 13:01:02 UTC) #62

hbos

Description was changed from ========== H.264 video codec support using OpenH264 (http://www.openh264.org/) for encoding and ...

4 years, 11 months ago (2016-01-21 14:39:23 UTC) #63

hbos

PTAL palmer. I addressed the buffer overflow memory issue and changed the build flag in ...

4 years, 11 months ago (2016-01-21 22:30:43 UTC) #68

hbos_chromium

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

4 years, 11 months ago (2016-01-21 22:31:48 UTC) #69

hbos_chromium

...and reply to comments, whoops. https://codereview.chromium.org/1306813009/diff/710001/webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right): https://codereview.chromium.org/1306813009/diff/710001/webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc#newcode90 webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:90: // cast. See https://bugs.chromium.org/p/webrtc/issues/detail?id=5428. ...

4 years, 11 months ago (2016-01-21 22:31:49 UTC) #70

...and reply to comments, whoops.

https://codereview.chromium.org/1306813009/diff/710001/webrtc/modules/video_c...
File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right):

https://codereview.chromium.org/1306813009/diff/710001/webrtc/modules/video_c...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:90: // cast. See
https://bugs.chromium.org/p/webrtc/issues/detail?id=5428.
On 2016/01/20 19:17:47, palmer wrote:
> Cool, thanks!
> 
> Nit: delineate identifiers with |...|.

Done. Question: Do you think it's important to avoid the "unsafe type cast"
though? Seems a bit silly if we trust FFmpeg; if it does what the API/doc says
then we always get what we expect, and Chromium's FFmpeg use does
reinterpret_cast. (I'm looking into if I can reuse an existing frame buffer pool
or if I need to write my own.)

https://codereview.chromium.org/1306813009/diff/710001/webrtc/modules/video_c...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:105: if (ret < 0) {
On 2016/01/20 19:17:47, palmer wrote:
> Looking at the implementation of |av_image_check_size|, 0 is the sole
> "everything is OK" return value. So you can make this check tighter by doing:
> 
>   if (ret) { ... }
> 
> or, if you like,
> 
>   if (0 != ret) { ... }

The doc says "@return >= 0 if valid, a negative error code otherwise". They
could introduce a wider variety of return values without changing the API. Or
someone reading the doc without looking at the impl might be confused why we do
!= 0 if I change this. I prefer < 0.

https://codereview.chromium.org/1306813009/diff/710001/webrtc/modules/video_c...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (right):

https://codereview.chromium.org/1306813009/diff/710001/webrtc/modules/video_c...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:118: for (int nal =
0; nal < layerInfo.iNalCount; ++nal, ++frag) {
On 2016/01/20 19:17:47, palmer wrote:
> If I am reading this code right, and I am not sure I am, |fragments_count| is
> guaranteed to equal info->iLayerNum, due to the loop in lines 79 – 90. Is that
> right?
> 
> But here, you increment |frag| layerInfo.iNalCount times. Is
layerInfo.iNalCount
> guaranteed to equal info->iLayerNum? Are there guaranteed to be
> layerInfo.iNalCount fragments in frag_header->fragmentationOffset?

Each layer contains a number of fragments. |fragment_count| is the total number
of fragments over all layers. (I.e. |fragment_count > iLayerNum| unless each
layer only contains a single fragment.)

Look again, I increment |fragment_count| the same way I increment |frag|. (Only
difference is if I do it in at the end of the for statement or at the end of the
body. Updating.)

https://codereview.chromium.org/1306813009/diff/710001/webrtc/modules/video_c...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:123:
frag_header->fragmentationOffset[frag] =
On 2016/01/20 19:17:47, palmer wrote:
> Can you add a comment explaining why the arithmetic in lines 123 – 127 is
> guaranteed not to overflow or underflow?

Done. Also since I DCHECK that it starts with a start code I added a DCHECK to
make sure the layer is at least as big as a start code.

sanmrtn96

sanmrtn96@gmail.com changed reviewers: + sanmrtn96@gmail.com

4 years, 11 months ago (2016-01-22 11:16:08 UTC) #71

sanmrtn96

I saw this comment in the encoder implementation which worries me a bit. Have you ...

4 years, 11 months ago (2016-01-22 11:16:08 UTC) #72

hbos

Description was changed from ========== H.264 video codec support using OpenH264 (http://www.openh264.org/) for encoding and ...

4 years, 11 months ago (2016-01-26 11:41:43 UTC) #73

hbos

PTAL palmer, all bots are green and I intend to land this shortly. (Defaulting h264 ...

4 years, 11 months ago (2016-01-26 12:47:36 UTC) #76

hbos

Description was changed from ========== H.264 video codec support using OpenH264 (http://www.openh264.org/) for encoding and ...

4 years, 11 months ago (2016-01-26 12:49:00 UTC) #77

sanmrtn96

On 2016/01/26 12:47:36, hbos wrote: > Looking at the source code, bIDR is ignored, it ...

4 years, 11 months ago (2016-01-26 13:34:41 UTC) #78

hbos

On 2016/01/26 13:34:41, sanmrtn96 wrote: > Thanks for testing. May I ask you: > > ...

4 years, 11 months ago (2016-01-26 16:24:48 UTC) #79

hbos

Description was changed from ========== H.264 video codec support using OpenH264 (http://www.openh264.org/) for encoding and ...

4 years, 11 months ago (2016-01-27 09:20:36 UTC) #80

hbos

Description was changed from ========== H.264 video codec support using OpenH264 (http://www.openh264.org/) for encoding and ...

4 years, 11 months ago (2016-01-27 09:28:01 UTC) #81

hbos

The patchset sent to the CQ was uploaded after l-g-t-m from noahric@chromium.org, stefan@webrtc.org, kjellander@webrtc.org, mflodman@webrtc.org ...

4 years, 11 months ago (2016-01-27 09:34:17 UTC) #83

commit-bot: I haz the power

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

4 years, 11 months ago (2016-01-27 09:34:21 UTC) #84

commit-bot: I haz the power

Description was changed from ========== H.264 video codec support using OpenH264 (http://www.openh264.org/) for encoding and ...

4 years, 11 months ago (2016-01-27 09:36:07 UTC) #85

commit-bot: I haz the power

Committed patchset #36 (id:1010001)

4 years, 11 months ago (2016-01-27 09:36:08 UTC) #86

commit-bot: I haz the power

Description was changed from ========== H.264 video codec support using OpenH264 (http://www.openh264.org/) for encoding and ...

4 years, 11 months ago (2016-01-27 09:36:15 UTC) #87

commit-bot: I haz the power

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

4 years, 11 months ago (2016-01-27 09:36:17 UTC) #88

hbos

On 2016/01/27 09:36:17, commit-bot: I haz the power wrote: > Patchset 36 (id:??) landed as ...

4 years, 11 months ago (2016-01-27 09:41:14 UTC) #89

zjx20

Hi hbos, sorry for asking you the silly questions: What is the specific limitation from ...

4 years, 11 months ago (2016-01-28 03:11:47 UTC) #90

hbos

On 2016/01/28 03:11:47, zjx20 wrote: > Hi hbos, sorry for asking you the silly questions: ...

4 years, 10 months ago (2016-02-05 10:39:59 UTC) #91

hbos

As for why we use two libraries, FFmpeg is already included in Chrome, so not ...

4 years, 10 months ago (2016-02-05 10:43:06 UTC) #92

zjx20

4 years, 10 months ago (2016-02-17 03:25:17 UTC) #93

Message was sent while issue was closed.

On 2016/02/05 10:43:06, hbos wrote:
> As for why we use two libraries, FFmpeg is already included in Chrome, so not
> using OpenH264 for decoding saves binary size. And reusing FFmpeg means
reusing
> what has already been tested and gone through reviews.

Thank you for the explanation!

Issue 1306813009: H.264 video codec support using OpenH264/FFmpeg (Closed)

Description

Patch Set 1 : Initial encoder/decoder implementations, scavanged from old hbos appspot CL #

Patch Set 2 : H264 in EndToEndTest.SendsAndReceivesH264 #

Patch Set 3 : H264 in video_loopback and bitrate,etc bugfixes #

Patch Set 4 : RTPDefragmentize, videoprocessor_unittest: H264 with no packet loss #

Patch Set 5 : H264 in full_stack (for full_stack_quality_sampler) and screenshare_loopback #

Patch Set 6 : OpenH264 behind compiler flag (gyp: use_openh264, #if: WEBRTC_OPENH264) #

Patch Set 7 : H264EncoderImpl: Reallocating EncodedImage buffer if necessary #

Patch Set 8 : Misc (WebRtcVideoChannel2::...::ConfigureVideoEncoderSettings care about H264 case) #

Patch Set 9 : Addressed noahric's comments. Handling kKeyFrame, kSkipFrame. #

Patch Set 10 : Addressed stefan's comments #

Patch Set 11 : Rebase with master. This undos my full_stack changes because of heavy refactoring (un-supporting H264 in video_loopback and screenshare_loopback) #

Patch Set 12 : Re-enable H264 in video_loopback and screenshare_loopback after rebase (video_quality_test) #

Patch Set 13 : Addressed stefan and hta comments #

Patch Set 14 : Rebase with master after some time #

Patch Set 15 : Addressed comments (type safety, overflow check) #

Patch Set 16 : Rebase with master #

Patch Set 17 : OpenH264 from third_party, H264DecoderImpl /w FFmpeg instead of OpenH264 #

Patch Set 18 : (temporary debug prints) #

Patch Set 19 : Rebase with master (and remove temporary debug prints) #

Patch Set 20 : GN build files and minor changes #

Patch Set 21 : sync_chromium.py SCRIPT_VERSION bumped ensuring ffmpeg is synced #

Patch Set 22 : Addressed noahric's comments + minor fixes #

Patch Set 23 : Addressed nits #

Patch Set 24 : Addressed comments #

Patch Set 25 : Rebase with master #

Patch Set 26 : Rebase with master #

Patch Set 27 : Rebase master (after other CL): rtc_use_h264 flag. Override its default to true for trybots. #

Patch Set 28 : Rebase master (after other CL): EncodedImage::GetBufferPaddingBytes #

Patch Set 29 : Padding = 8, fixing ffmpeg overreading input buffer memory issue #

Patch Set 30 : Applied other CL patch (use padding fix). Encoder output CodecSpecificInfo to Encoded cb. #

Patch Set 31 : Addressed comments #

Patch Set 32 : Rebase with master. H264 disabled on android. #

Patch Set 33 : Fixed memory issue (avcodec_align_dimensions + crop) #

Patch Set 34 : Rebase with master, updated comment #

Patch Set 35 : Rebase with master, flags off my default #

Patch Set 36 : (Alphabetical sorting in common_video.gyp deps) #

Messages