Initialize packetization mode in VideoToolbox

Initialize packetization mode in VideoToolbox

This fixes a recently introduced bug where we would read uninitialized
memory as the packetization_mode member in codec_specific_info was not
being properly initialized.

BUG=webrtc:6858
Review-Url: https://codereview.webrtc.org/2577043003
Cr-Commit-Position: refs/heads/master@{#15646}
Committed: https://chromium.googlesource.com/external/webrtc/+/05d6e260f598f8eabcca16c63e53d1b3dd412dd2

[kthelgason, 2016-12-15 14:09:22]

kthelgason@webrtc.org changed reviewers:
+ hta@webrtc.org, magjed@webrtc.org, tkchin@webrtc.org

[hta-webrtc, 2016-12-15 14:29:01]

lgtm

This should fix the problem, but there's an underlying problem that needs
fixing; suggest leaving a TODO or two in the code to point out to the next
maintainer that there's an issue here.

https://codereview.webrtc.org/2577043003/diff/20001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/h264_video_toolbox_encoder.mm (right):

https://codereview.webrtc.org/2577043003/diff/20001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/h264_video_toolbox_encoder.mm:379: const
CodecSpecificInfo* codec_specific_info,
The next-level problem is probably that this parameter is garbage (if I read the
code right), since we have to overwrite it below.

https://codereview.webrtc.org/2577043003/diff/20001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/h264_video_toolbox_encoder.mm:476:
packetization_mode_;
This is OK as a first-order fix; if internal::FrameEncodeParams' constructor is
working in the obvious way, we're overwriting a value copied from the
codec_specific_info parameter passed to this function.

It makes sense to add a TODO here.

[kthelgason, 2016-12-15 14:38:13]

https://codereview.webrtc.org/2577043003/diff/20001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/h264_video_toolbox_encoder.mm (right):

https://codereview.webrtc.org/2577043003/diff/20001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/h264_video_toolbox_encoder.mm:379: const
CodecSpecificInfo* codec_specific_info,
On 2016/12/15 14:29:00, hta-webrtc wrote:
> The next-level problem is probably that this parameter is garbage (if I read
the
> code right), since we have to overwrite it below.

I think this param is always NULL, at least when I inspected it while running. I
haven't traced where it comes from though.

https://codereview.webrtc.org/2577043003/diff/20001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/h264_video_toolbox_encoder.mm:476:
packetization_mode_;
On 2016/12/15 14:29:01, hta-webrtc wrote:
> This is OK as a first-order fix; if internal::FrameEncodeParams' constructor
is
> working in the obvious way, we're overwriting a value copied from the
> codec_specific_info parameter passed to this function.
> 
> It makes sense to add a TODO here.

In the implementation of this method in h264_encoder_impl.cc this param goes
unused. I think it's value when passed in is not important.

[tkchin_webrtc, 2016-12-15 20:35:35]

On 2016/12/15 14:38:13, kthelgason wrote:
>
https://codereview.webrtc.org/2577043003/diff/20001/webrtc/sdk/objc/Framework...
> File webrtc/sdk/objc/Framework/Classes/h264_video_toolbox_encoder.mm (right):
> 
>
https://codereview.webrtc.org/2577043003/diff/20001/webrtc/sdk/objc/Framework...
> webrtc/sdk/objc/Framework/Classes/h264_video_toolbox_encoder.mm:379: const
> CodecSpecificInfo* codec_specific_info,
> On 2016/12/15 14:29:00, hta-webrtc wrote:
> > The next-level problem is probably that this parameter is garbage (if I read
> the
> > code right), since we have to overwrite it below.
> 
> I think this param is always NULL, at least when I inspected it while running.
I
> haven't traced where it comes from though.
> 
>
https://codereview.webrtc.org/2577043003/diff/20001/webrtc/sdk/objc/Framework...
> webrtc/sdk/objc/Framework/Classes/h264_video_toolbox_encoder.mm:476:
> packetization_mode_;
> On 2016/12/15 14:29:01, hta-webrtc wrote:
> > This is OK as a first-order fix; if internal::FrameEncodeParams' constructor
> is
> > working in the obvious way, we're overwriting a value copied from the
> > codec_specific_info parameter passed to this function.
> > 
> > It makes sense to add a TODO here.
> 
> In the implementation of this method in h264_encoder_impl.cc this param goes
> unused. I think it's value when passed in is not important.

When was this introduced? Does it affect H264 high profile?

[tkchin_webrtc, 2016-12-15 20:35:45]

On 2016/12/15 20:35:35, tkchin_webrtc wrote:
> On 2016/12/15 14:38:13, kthelgason wrote:
> >
>
https://codereview.webrtc.org/2577043003/diff/20001/webrtc/sdk/objc/Framework...
> > File webrtc/sdk/objc/Framework/Classes/h264_video_toolbox_encoder.mm
(right):
> > 
> >
>
https://codereview.webrtc.org/2577043003/diff/20001/webrtc/sdk/objc/Framework...
> > webrtc/sdk/objc/Framework/Classes/h264_video_toolbox_encoder.mm:379: const
> > CodecSpecificInfo* codec_specific_info,
> > On 2016/12/15 14:29:00, hta-webrtc wrote:
> > > The next-level problem is probably that this parameter is garbage (if I
read
> > the
> > > code right), since we have to overwrite it below.
> > 
> > I think this param is always NULL, at least when I inspected it while
running.
> I
> > haven't traced where it comes from though.
> > 
> >
>
https://codereview.webrtc.org/2577043003/diff/20001/webrtc/sdk/objc/Framework...
> > webrtc/sdk/objc/Framework/Classes/h264_video_toolbox_encoder.mm:476:
> > packetization_mode_;
> > On 2016/12/15 14:29:01, hta-webrtc wrote:
> > > This is OK as a first-order fix; if internal::FrameEncodeParams'
constructor
> > is
> > > working in the obvious way, we're overwriting a value copied from the
> > > codec_specific_info parameter passed to this function.
> > > 
> > > It makes sense to add a TODO here.
> > 
> > In the implementation of this method in h264_encoder_impl.cc this param goes
> > unused. I think it's value when passed in is not important.
> 
> When was this introduced? Does it affect H264 high profile?

rs lgtm

[magjed_webrtc, 2016-12-16 09:39:49]

On 2016/12/15 20:35:35, tkchin_webrtc wrote:
> On 2016/12/15 14:38:13, kthelgason wrote:
> >
>
https://codereview.webrtc.org/2577043003/diff/20001/webrtc/sdk/objc/Framework...
> > File webrtc/sdk/objc/Framework/Classes/h264_video_toolbox_encoder.mm
(right):
> > 
> >
>
https://codereview.webrtc.org/2577043003/diff/20001/webrtc/sdk/objc/Framework...
> > webrtc/sdk/objc/Framework/Classes/h264_video_toolbox_encoder.mm:379: const
> > CodecSpecificInfo* codec_specific_info,
> > On 2016/12/15 14:29:00, hta-webrtc wrote:
> > > The next-level problem is probably that this parameter is garbage (if I
read
> > the
> > > code right), since we have to overwrite it below.
> > 
> > I think this param is always NULL, at least when I inspected it while
running.
> I
> > haven't traced where it comes from though.
> > 
> >
>
https://codereview.webrtc.org/2577043003/diff/20001/webrtc/sdk/objc/Framework...
> > webrtc/sdk/objc/Framework/Classes/h264_video_toolbox_encoder.mm:476:
> > packetization_mode_;
> > On 2016/12/15 14:29:01, hta-webrtc wrote:
> > > This is OK as a first-order fix; if internal::FrameEncodeParams'
constructor
> > is
> > > working in the obvious way, we're overwriting a value copied from the
> > > codec_specific_info parameter passed to this function.
> > > 
> > > It makes sense to add a TODO here.
> > 
> > In the implementation of this method in h264_encoder_impl.cc this param goes
> > unused. I think it's value when passed in is not important.
> 
> When was this introduced? Does it affect H264 high profile?

It's unrelated to the profiles and should not affect high profile. The
packetization mode problem was introduced in "Reland of H.264 packetization mode
0" https://codereview.webrtc.org/2558463002/.

[magjed_webrtc, 2016-12-16 09:40:06]

The CQ bit was checked by magjed@webrtc.org

[magjed_webrtc, 2016-12-16 09:40:08]

lgtm

[commit-bot: I haz the power, 2016-12-16 09:40:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-12-16 10:10:20]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481881206935480,
"parent_rev": "9a272059c2617ef0dabf2b6c69649957bcbcc027", "commit_rev":
"05d6e260f598f8eabcca16c63e53d1b3dd412dd2"}

[commit-bot: I haz the power, 2016-12-16 10:10:22]

Description was changed from

==========
Initialize packetization mode in VideoToolbox

This fixes a recently introduced bug where we would read uninitialized
memory as the packetization_mode member in codec_specific_info was not
being properly initialized.

BUG=webrtc:6858
==========

to

==========
Initialize packetization mode in VideoToolbox

This fixes a recently introduced bug where we would read uninitialized
memory as the packetization_mode member in codec_specific_info was not
being properly initialized.

BUG=webrtc:6858

Review-Url: https://codereview.webrtc.org/2577043003
Cr-Commit-Position: refs/heads/master@{#15646}
Committed:
https://chromium.googlesource.com/external/webrtc/+/05d6e260f598f8eabcca16c63...
==========

[commit-bot: I haz the power, 2016-12-16 10:10:23]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/external/webrtc/+/05d6e260f598f8eabcca16c63...