Implement ISO BMFF support in Media Source.

media/mp4/avc.h

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef MEDIA_MP4_AVC_H_
+#define MEDIA_MP4_AVC_H_
+
+#include "base/basictypes.h"
+#include "media/base/channel_layout.h"
+#include <vector>
+
+namespace media {
+namespace mp4 {
+
+struct AVCDecoderConfigurationRecord {
+  uint8 version;

[acolwell GONE FROM CHROMIUM, 2012/06/06 17:32:39]

nit: use int here and below. In general Chrome prefers use of regular int
instead of unsigned types and assumes int is at least 32 bits.

[strobe_, 2012/06/07 16:33:54]

I'm working on converting the polymorphic Read() functions into separate calls
for each site to make the structure of the box more visible within the parsing
functions, but I'm not in favor of arbitrarily destroying the type symmetry
between the spec and the header by making everything an int. Here's why:

Currently, the process for adding or verifying a box is:

- Check to make sure that the type names in the struct match those in the spec
- Check to make sure the call to Read() is in the right spot
- Debugging? Just print the bytes of the structure and find out where you
misregistered. 

There are a few cases where the logic gets more complex, but for most fields,
the two-step verification is all that's required. It seems to work well, too; we
had relatively few bugs in the parsing code after writing it.

Using ints wherever possible would alter this process:

- Look at the source type, and mentally translate it to the destination type.
  - Is it a uint32? Decide whether it's reasonable to assume that the value will
always fit in the positive half of an int32. If it doesn't, decide whether to
stick it in an int64 or a uint32. If it does, maybe write a test or add an
RCHECK/DCHECK at the read site to ensure that it does, unless we decide the
parameter is one we don't actually care that much about. Unless we might ever do
a right shift on it, in which case we have to be careful to cast to a uint
before shifting or we'll get sign-extended goop in the higher bits.
  - Is it smaller than an int32? Then use one. Be sure to do sign extension,
unless the value is a flag in which case be sure to avoid it.
  - Is it a uint64? Should we try to pack it into an int64 for consistency and
to avoid unsignedness? Can we actually reasonably count on it fitting on a
32-bit int, and just fail files that don't?
- Decide whether there was semantic value in the original type, and maybe add
some comments around usage sites. Maybe record the rationale of your decision.
- Write the parsing line for that, choosing from among the 20-odd necessary
permutations of the read function [source size, source signedness, dest size,
dest signedness], followed by any verification checks needed to ensure that you
made the right packing decision.
- After you're finished writing all the parsing functions, go back through and
examine them to make sure that all implicitly cross-referenced values (such as
track IDs on separate boxes) were chosen to have the same type on the host, and
that they are always parsed and used in the same way. Add more comments whenever
you get burned by a mismatch.
- Debugging? Be sure to have the spec, header, and implementation files open to
reverse-engineer the layout changes and determine which bytes made it into what
values in which order.

Now of course this mental loop isn't *that* hard, but it's still much more than
is needed when just copying types from the spec. The point of consistency in
coding style is to make code easier to understand and maintain, and I think that
in this case, keeping consistency with the spec does that better than keeping
consistency with code in other modules.

Thoughts?

[acolwell GONE FROM CHROMIUM, 2012/06/07 19:44:02]

Take it easy. This is the first file I reviewed and didn't realize that this was
a format specific structure. I agree that it makes sense to keep these matching
the spec which is why you don't see as many of these "use int" comments later. 
In cases like simple iteration on quantities known to be smaller than 32 bits
int is preferred.

[strobe_, 2012/06/07 22:04:22]

Contritely: I apologize. I see that that message may have come off as more
hostile than it was intended to be. 

With wry humor: I think the Elcor might have had the right idea. For email, at
least.

Suddenly concerned: Oh, I hope you've played Mass Effect, because if you
haven't, this joke won't land at all.
Got it, I'll give everything another pass to see where I can apply this without
issue.

+  uint8 profile_indication;
+  uint8 profile_compatibility;
+  uint8 avc_level;
+  uint8 length_size;
+
+  uint32 width;
+  uint32 height;
+  uint32 crop_top;
+  uint32 crop_left;
+  uint32 crop_right;
+  uint32 crop_bottom;
+
+  typedef std::vector<uint8> SPS;
+  typedef std::vector<uint8> PPS;
+
+  std::vector<SPS> spses;

[acolwell GONE FROM CHROMIUM, 2012/06/06 17:32:39]

nit: How about sps_list & pps_list? or xxx_vector?

[strobe_, 2012/06/07 16:33:54]

Done.

+  std::vector<PPS> ppses;
+
+  AVCDecoderConfigurationRecord();
+  ~AVCDecoderConfigurationRecord();
+};
+
+bool ConvertAVCCToAnnexB(uint8 length_size, std::vector<uint8>* buffer);
+
+ChannelLayout ConvertAACChannelCountToChannelLayout(uint16 count);

[acolwell GONE FROM CHROMIUM, 2012/06/06 17:32:39]

nit: change count to int

[strobe_, 2012/06/07 16:33:54]

Done.

+
+}  // namespace mp4
+}  // namespace media
+
+#endif  // MEDIA_MP4_AVC_H_