VP8Parser: Fix calculation of DCT partition sizes.

VP8Parser: Fix calculation of DCT partition sizes.

Partition sizes reside all together after the first partition in the streaim,
not at the beginning of each DCT partition.
Also, fix an erroneous DCHECK in V4L2SVDA.

BUG=chrome-os-partner:36432
TEST=media_unittest for VP8Parser,VP8 playback,vdaunittest

Committed: https://crrev.com/3c97550d09b3d4b9fc0d81b5a35437e6e211a581
Cr-Commit-Position: refs/heads/master@{#316647}

[Pawel Osciak, 2015-02-13 11:53:43]

posciak@chromium.org changed reviewers:
+ vmr@chromium.org, wuchengli@chromium.org

[Pawel Osciak, 2015-02-13 11:53:49]

ptal

[Ville-Mikko Rautio, 2015-02-13 23:03:02]

https://chromiumcodereview.appspot.com/923033002/diff/1/media/filters/vp8_par...
File media/filters/vp8_parser.cc (right):

https://chromiumcodereview.appspot.com/923033002/diff/1/media/filters/vp8_par...
media/filters/vp8_parser.cc:813: fhdr->data + fhdr->first_part_offset +
fhdr->first_part_size;
To minimize the chance of confusion it might be worth considering of using
exclusively either full name (partition) or just the acronym (part) in each
field of Vp8FrameHeader when referring to partitions.

https://chromiumcodereview.appspot.com/923033002/diff/1/media/filters/vp8_par...
media/filters/vp8_parser.cc:817: fhdr->dct_partition_sizes[i] = (ptr[2] << 16) |
(ptr[1] << 8) | ptr[0];
IIUC ptr[i] (i>=0) might refer out of allocated bounds, if frame buffer ends for
some reason right after previous partition. After all, num_of_dct_partitions
might be even faulty. Restore the if to prevent this or otherwise make sure
there will be at least three bytes left to read from ptr at this stage.

[Pawel Osciak, 2015-02-13 23:07:22]

https://chromiumcodereview.appspot.com/923033002/diff/1/media/filters/vp8_par...
File media/filters/vp8_parser.cc (right):

https://chromiumcodereview.appspot.com/923033002/diff/1/media/filters/vp8_par...
media/filters/vp8_parser.cc:813: fhdr->data + fhdr->first_part_offset +
fhdr->first_part_size;
On 2015/02/13 23:03:02, Ville-Mikko Rautio wrote:
> To minimize the chance of confusion it might be worth considering of using
> exclusively either full name (partition) or just the acronym (part) in each
> field of Vp8FrameHeader when referring to partitions.

So you mean change num_of_dct_partitions to num_of_dct_parts?

https://chromiumcodereview.appspot.com/923033002/diff/1/media/filters/vp8_par...
media/filters/vp8_parser.cc:817: fhdr->dct_partition_sizes[i] = (ptr[2] << 16) |
(ptr[1] << 8) | ptr[0];
On 2015/02/13 23:03:02, Ville-Mikko Rautio wrote:
> IIUC ptr[i] (i>=0) might refer out of allocated bounds, if frame buffer ends
for
> some reason right after previous partition. After all, num_of_dct_partitions
> might be even faulty. Restore the if to prevent this or otherwise make sure
> there will be at least three bytes left to read from ptr at this stage.

I check this at l.801 above. I check that frame_size is at least enough to get
us to the beginning of first DCT partition, which is after first partition and
after (fhdr->num_of_dct_partitions - 1) * 3 bytes of sizes. So if
num_of_dct_partitions is faulty, that should catch it. Unless I'm missing
something?

[Ville-Mikko Rautio, 2015-02-13 23:17:38]

LGTM

https://chromiumcodereview.appspot.com/923033002/diff/1/media/filters/vp8_par...
File media/filters/vp8_parser.cc (right):

https://chromiumcodereview.appspot.com/923033002/diff/1/media/filters/vp8_par...
media/filters/vp8_parser.cc:813: fhdr->data + fhdr->first_part_offset +
fhdr->first_part_size;
On 2015/02/13 23:07:21, Pawel Osciak wrote:
> On 2015/02/13 23:03:02, Ville-Mikko Rautio wrote:
> > To minimize the chance of confusion it might be worth considering of using
> > exclusively either full name (partition) or just the acronym (part) in each
> > field of Vp8FrameHeader when referring to partitions.
> 
> So you mean change num_of_dct_partitions to num_of_dct_parts?

Either that or change (first_part_offset, first_part_size) to
(first_partition_offset, first_partition_size).

https://chromiumcodereview.appspot.com/923033002/diff/1/media/filters/vp8_par...
media/filters/vp8_parser.cc:817: fhdr->dct_partition_sizes[i] = (ptr[2] << 16) |
(ptr[1] << 8) | ptr[0];
On 2015/02/13 23:07:21, Pawel Osciak wrote:
> On 2015/02/13 23:03:02, Ville-Mikko Rautio wrote:
> > IIUC ptr[i] (i>=0) might refer out of allocated bounds, if frame buffer ends
> for
> > some reason right after previous partition. After all, num_of_dct_partitions
> > might be even faulty. Restore the if to prevent this or otherwise make sure
> > there will be at least three bytes left to read from ptr at this stage.
> 
> I check this at l.801 above. I check that frame_size is at least enough to get
> us to the beginning of first DCT partition, which is after first partition and
> after (fhdr->num_of_dct_partitions - 1) * 3 bytes of sizes. So if
> num_of_dct_partitions is faulty, that should catch it. Unless I'm missing
> something?

You're correct. Also on line 821 you'll exit if there's not enough data for next
partition.

[Pawel Osciak, 2015-02-13 23:20:39]

https://chromiumcodereview.appspot.com/923033002/diff/1/media/filters/vp8_par...
File media/filters/vp8_parser.cc (right):

https://chromiumcodereview.appspot.com/923033002/diff/1/media/filters/vp8_par...
media/filters/vp8_parser.cc:813: fhdr->data + fhdr->first_part_offset +
fhdr->first_part_size;
On 2015/02/13 23:17:38, Ville-Mikko Rautio wrote:
> On 2015/02/13 23:07:21, Pawel Osciak wrote:
> > On 2015/02/13 23:03:02, Ville-Mikko Rautio wrote:
> > > To minimize the chance of confusion it might be worth considering of using
> > > exclusively either full name (partition) or just the acronym (part) in
each
> > > field of Vp8FrameHeader when referring to partitions.
> > 
> > So you mean change num_of_dct_partitions to num_of_dct_parts?
> 
> Either that or change (first_part_offset, first_part_size) to
> (first_partition_offset, first_partition_size).

I see, makes sense. Given that this will have to be merged, I'll keep things as
is for now and consider this for later changes. Thanks.

[Pawel Osciak, 2015-02-13 23:22:07]

posciak@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[Pawel Osciak, 2015-02-13 23:22:08]

Dale: may I please have an OWNERS for media/ ? Thanks.

[DaleCurtis, 2015-02-17 19:53:01]

lgtm

[Pawel Osciak, 2015-02-17 20:37:33]

The CQ bit was checked by posciak@chromium.org

[commit-bot: I haz the power, 2015-02-17 20:38:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/923033002/1

[commit-bot: I haz the power, 2015-02-17 20:42:31]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-02-17 20:43:16]

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