H264SPS: Centralize computation of coded size and visible rect.

H264SPS: Centralize computation of coded size and visible rect.

These new methods are paranoid about undefined behavior, and they
replace similar code in H264Decoder and EsParserH264.

BUG=639128
Committed: https://crrev.com/d49079c5e9d04760afb4ffb433d11e8006c3e81a
Cr-Commit-Position: refs/heads/master@{#415488}

[sandersd (OOO until July 31), 2016-08-26 02:05:54]

sandersd@chromium.org changed reviewers:
+ posciak@chromium.org, servolk@chromium.org

[sandersd (OOO until July 31), 2016-08-26 02:05:54]

Unit tests will be completed tomorrow; but I'd like early review of the API
design if possible. Thanks!

[servolk, 2016-08-26 16:41:30]

On 2016/08/26 02:05:54, sandersd wrote:
> Unit tests will be completed tomorrow; but I'd like early review of the API
> design if possible. Thanks!

lgtm. thanks!

[servolk, 2016-08-26 16:43:05]

https://codereview.chromium.org/2268183009/diff/1/media/filters/h264_parser.cc
File media/filters/h264_parser.cc (right):

https://codereview.chromium.org/2268183009/diff/1/media/filters/h264_parser.c...
media/filters/h264_parser.cc:47: // Spec section 7.4.2.1.1.
Nit: I think we could be a bit more explicit about what spec is meant here:
// Spec: ISO/IEC 14496-10 section 7.4.2.1.1.

[sandersd (OOO until July 31), 2016-08-26 18:23:40]

Description was changed from

==========
H264SPS: Centralize comutation of coded size and visible rect.

These new methods are paranoid about undefined behavior, and they
replace similar code in H264Decoder and EsParserH264.

BUG=639128
==========

to

==========
H264SPS: Centralize computation of coded size and visible rect.

These new methods are paranoid about undefined behavior, and they
replace similar code in H264Decoder and EsParserH264.

BUG=639128
==========

[sandersd (OOO until July 31), 2016-08-27 01:11:01]

PTAL.

I've added tests, but decided to leave them minimal since I couldn't find a way
to write them that wouldn't be broken in the same ways I could have
misunderstood the spec while writing the code.

Changed the methods to be const since GetSPS() returns a const pointer. This
prevents us from adding caching, but I doubt this is a serious performance
concern.

https://codereview.chromium.org/2268183009/diff/1/media/filters/h264_parser.cc
File media/filters/h264_parser.cc (right):

https://codereview.chromium.org/2268183009/diff/1/media/filters/h264_parser.c...
media/filters/h264_parser.cc:47: // Spec section 7.4.2.1.1.
On 2016/08/26 16:43:05, servolk wrote:
> Nit: I think we could be a bit more explicit about what spec is meant here:
> // Spec: ISO/IEC 14496-10 section 7.4.2.1.1.

Done.

[sandersd (OOO until July 31), 2016-08-29 22:41:14]

The CQ bit was checked by sandersd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-29 23:19:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 03:28:45]

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

[commit-bot: I haz the power, 2016-08-30 03:28:46]

Dry run: This issue passed the CQ dry run.

[sandersd (OOO until July 31), 2016-08-30 17:39:21]

On 2016/08/27 01:11:01, sandersd wrote:
> PTAL.
> 
> I've added tests, but decided to leave them minimal since I couldn't find a
way
> to write them that wouldn't be broken in the same ways I could have
> misunderstood the spec while writing the code.
> 
> Changed the methods to be const since GetSPS() returns a const pointer. This
> prevents us from adding caching, but I doubt this is a serious performance
> concern.
> 
> https://codereview.chromium.org/2268183009/diff/1/media/filters/h264_parser.cc
> File media/filters/h264_parser.cc (right):
> 
>
https://codereview.chromium.org/2268183009/diff/1/media/filters/h264_parser.c...
> media/filters/h264_parser.cc:47: // Spec section 7.4.2.1.1.
> On 2016/08/26 16:43:05, servolk wrote:
> > Nit: I think we could be a bit more explicit about what spec is meant here:
> > // Spec: ISO/IEC 14496-10 section 7.4.2.1.1.
> 
> Done.

posciak@: ping.

servolk@: ping, if you want to look at the tests (I'll assume no at EOD today).

[sandersd (OOO until July 31), 2016-08-30 23:12:52]

On 2016/08/30 17:39:21, sandersd wrote:
> On 2016/08/27 01:11:01, sandersd wrote:
> > PTAL.
> > 
> > I've added tests, but decided to leave them minimal since I couldn't find a
> way
> > to write them that wouldn't be broken in the same ways I could have
> > misunderstood the spec while writing the code.
> > 
> > Changed the methods to be const since GetSPS() returns a const pointer. This
> > prevents us from adding caching, but I doubt this is a serious performance
> > concern.
> > 
> >
https://codereview.chromium.org/2268183009/diff/1/media/filters/h264_parser.cc
> > File media/filters/h264_parser.cc (right):
> > 
> >
>
https://codereview.chromium.org/2268183009/diff/1/media/filters/h264_parser.c...
> > media/filters/h264_parser.cc:47: // Spec section 7.4.2.1.1.
> > On 2016/08/26 16:43:05, servolk wrote:
> > > Nit: I think we could be a bit more explicit about what spec is meant
here:
> > > // Spec: ISO/IEC 14496-10 section 7.4.2.1.1.
> > 
> > Done.
> 
> posciak@: ping.
> 
> servolk@: ping, if you want to look at the tests (I'll assume no at EOD
today).

I'm going to commit this now, since there is a CF bug associated. If there are
any issues with the API we can worry about that later.

[sandersd (OOO until July 31), 2016-08-30 23:12:56]

The CQ bit was checked by sandersd@chromium.org

[sandersd (OOO until July 31), 2016-08-30 23:12:56]

The patchset sent to the CQ was uploaded after l-g-t-m from servolk@chromium.org
Link to the patchset: https://codereview.chromium.org/2268183009/#ps40001
(title: "Make sure fuzzing code actually runs.")

[commit-bot: I haz the power, 2016-08-30 23:13:29]

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

[commit-bot: I haz the power, 2016-08-30 23:20:31]

Description was changed from

==========
H264SPS: Centralize computation of coded size and visible rect.

These new methods are paranoid about undefined behavior, and they
replace similar code in H264Decoder and EsParserH264.

BUG=639128
==========

to

==========
H264SPS: Centralize computation of coded size and visible rect.

These new methods are paranoid about undefined behavior, and they
replace similar code in H264Decoder and EsParserH264.

BUG=639128
==========

[commit-bot: I haz the power, 2016-08-30 23:20:32]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-30 23:22:31]

Description was changed from

==========
H264SPS: Centralize computation of coded size and visible rect.

These new methods are paranoid about undefined behavior, and they
replace similar code in H264Decoder and EsParserH264.

BUG=639128
==========

to

==========
H264SPS: Centralize computation of coded size and visible rect.

These new methods are paranoid about undefined behavior, and they
replace similar code in H264Decoder and EsParserH264.

BUG=639128

Committed: https://crrev.com/d49079c5e9d04760afb4ffb433d11e8006c3e81a
Cr-Commit-Position: refs/heads/master@{#415488}
==========

[commit-bot: I haz the power, 2016-08-30 23:22:33]

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