Simple PMP reader to parse Picasa's metadata

webkit/fileapi/media/picasa/pmp_column_reader.cc

+// Copyright 2013 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.
+
+#include "webkit/fileapi/media/picasa/pmp_column_reader.h"
+
+#include <cstring>
+
+#include "base/files/file_path.h"
+#include "base/files/memory_mapped_file.h"
+#include "base/logging.h"
+#include "webkit/fileapi/media/picasa/pmp_constants.h"
+
+namespace fileapi {
+
+namespace {
+
+const size_t kPmpMaxFilesize = 50*1024*1024; // Maximum of 50 MB.
+
+}  // namespace
+
+PmpColumnReader::PmpColumnReader(std::string column_name)
+    : column_name_(column_name),
+      data_(NULL),
+      length_(0),
+      rows_(0),
+      strings_(),
+      fieldtype_(kPmpFieldTypeInvalid) { }
+
+PmpColumnReader::~PmpColumnReader() { }
+
+bool PmpColumnReader::ReadFromFile(const base::FilePath& filepath,
+                                   uint32* rows_read) {
+  base::MemoryMappedFile file;
+
+  if(!file.Initialize(filepath) ||
+     file.length() < kPmpHeaderSize ||
+     file.length() > kPmpMaxFilesize ||
+     file.data() == NULL) {
+    return false;
+  }
+
+  length_ = file.length();
+  data_.reset(new uint8[length_]);
+  std::copy(file.data(), file.data() + length_, data_.get());
+
+  return ParseData(rows_read);
+}
+
+bool PmpColumnReader::ReadString(const uint32 row, std::string* result) const {
+  DCHECK(data_.get() != NULL);
+  DCHECK(length_ > kPmpHeaderSize);

[vandebo (ex-Chrome), 2013/03/28 00:56:04]

DCHECK_GT

[tommycli, 2013/03/29 00:16:01]

Done.

+
+  if(fieldtype_ == kPmpFieldTypeString && row < rows_) {
+    *result = strings_[row];
+    return true;
+  } else {

[vandebo (ex-Chrome), 2013/03/28 00:56:04]

Remove else and just return false at the end.

[tommycli, 2013/03/29 00:16:01]

Done.

+    return false;
+  }
+}
+
+bool PmpColumnReader::ReadUInt32(const uint32 row, uint32* result) const {
+  return (fieldtype_ == kPmpFieldTypeUInt32 &&
+          ReadPrimitive<uint32>(row, result));

[vandebo (ex-Chrome), 2013/03/28 00:56:04]

Once you get everything else taken care, can you try to implement all this stuff
without any templates?

[tommycli, 2013/03/29 00:16:01]

Since we are only using 4 field types, the best alternative to the template was
to just copy-paste the template body and eliminate the template.

Kinda worried about the copy-paste though - what do you think?

[vandebo (ex-Chrome), 2013/03/29 21:35:07]

I think the amount of code duplication is small and it makes it much easier to
understand what's going on, so an overall win.

[tommycli, 2013/04/02 21:12:36]

Done.

+}
+
+bool PmpColumnReader::ReadDouble64(const uint32 row, double* result) const {
+  return (fieldtype_ == kPmpFieldTypeDouble64 &&
+          ReadPrimitive<double>(row, result));
+}
+
+bool PmpColumnReader::ReadUInt8(const uint32 row, uint8* result) const {
+  return (fieldtype_ == kPmpFieldTypeUInt8 &&
+          ReadPrimitive<uint8>(row, result));
+}
+
+bool PmpColumnReader::ReadUInt64(const uint32 row, uint64* result) const {
+  return (fieldtype_ == kPmpFieldTypeUInt64 &&
+          ReadPrimitive<uint64>(row, result));
+}
+
+template<class T>
+bool PmpColumnReader::ReadPrimitive(uint32 row, T* result) const {
+  DCHECK(data_.get() != NULL);
+  DCHECK(length_ > kPmpHeaderSize + row*sizeof(T));
+
+  if(row < rows_) {
+    *result = *(reinterpret_cast<T*>(

[vandebo (ex-Chrome), 2013/03/28 00:56:04]

Is this simpler?
*result = reinterpret_cast<T*>(data_.get() + kPmPHeaderSize)[row];

[tommycli, 2013/03/29 00:16:01]

Yeah, that's way better thanks.

+        data_.get() + kPmpHeaderSize + row*sizeof(T)));
+    return true;
+  } else {

[vandebo (ex-Chrome), 2013/03/28 00:56:04]

remove else. and return false at the end.

[tommycli, 2013/03/29 00:16:01]

Done.

+    return false;
+  }
+}
+
+bool PmpColumnReader::ParseData(uint32* rows_read) {
+  DCHECK(data_.get() != NULL);
+  DCHECK(length_ >= kPmpHeaderSize);
+
+  // Check all magic bytes.
+  if (memcmp(&kPmpMagic1, &data_[kPmpMagic1Offset], sizeof(kPmpMagic1)) != 0 ||

[vandebo (ex-Chrome), 2013/03/28 00:56:04]

data_.get() + kPmpMagic1Offset ?

[tommycli, 2013/03/29 00:16:01]

Makes the command no longer fit on one line. Change still desirable?

[vandebo (ex-Chrome), 2013/03/29 21:35:07]

Your call.

[tommycli, 2013/04/02 21:12:36]

Done.

+      memcmp(&kPmpMagic2, &data_[kPmpMagic2Offset], sizeof(kPmpMagic2)) != 0 ||
+      memcmp(&kPmpMagic3, &data_[kPmpMagic3Offset], sizeof(kPmpMagic3)) != 0 ||
+      memcmp(&kPmpMagic4, &data_[kPmpMagic4Offset], sizeof(kPmpMagic4)) != 0) {
+    return false;
+  }
+
+  // Verify row field type matches.
+  if (data_[kPmpFieldType1Offset] != data_[kPmpFieldType2Offset]) {

[vandebo (ex-Chrome), 2013/03/28 00:56:04]

This only tests the first byte of these two fields.  Is the type a uint16 ?

[vandebo (ex-Chrome), 2013/03/28 00:56:04]

nit: extra {}'s

[tommycli, 2013/03/29 00:16:01]

Whoops, that's just a straight up bug. I've refactored the field_type_ handling
to treat it as a uint16.

[tommycli, 2013/03/29 00:16:01]

Done.

+    return false;
+  }
+
+  // Read the field type and number of rows expected.
+  fieldtype_ = data_[kPmpFieldType1Offset];
+  rows_ = *(reinterpret_cast<uint32*>(&data_[kPmpRowCountOffset]));
+
+  bool parse_success = false;
+
+  switch (fieldtype_) {
+    case kPmpFieldTypeString:
+      parse_success = IndexStrings();
+      break;
+    case kPmpFieldTypeUInt32:
+      parse_success = ValidatePrimitiveArrayLength<uint32>();
+      break;
+    case kPmpFieldTypeDouble64:
+      parse_success = ValidatePrimitiveArrayLength<double>();
+      break;
+    case kPmpFieldTypeUInt8:
+      parse_success = ValidatePrimitiveArrayLength<uint8>();
+      break;
+    case kPmpFieldTypeUInt64:
+      parse_success = ValidatePrimitiveArrayLength<uint64>();
+      break;
+    default:
+      parse_success = false;

[vandebo (ex-Chrome), 2013/03/28 00:56:04]

return false;

[tommycli, 2013/03/29 00:16:01]

Done.

+      break;
+  }
+
+  if(parse_success) {
+    // Set the number of rows read. Though it's not valid unless we return true.
+    if(rows_read != NULL) {

[vandebo (ex-Chrome), 2013/03/28 00:56:04]

nit: extra {}'s

[tommycli, 2013/03/29 00:16:01]

Done.

+      *rows_read = rows_;
+    }
+    return true;
+  }
+
+  return false;
+}
+
+bool PmpColumnReader::IndexStrings() {
+  DCHECK(data_.get() != NULL);
+  DCHECK(length_ >= kPmpHeaderSize);

[vandebo (ex-Chrome), 2013/03/28 00:56:04]

DCHECK_GE

[tommycli, 2013/03/29 00:16:01]

Done.

+
+  strings_.clear();
+  strings_.reserve(rows_);
+
+  size_t bytes_parsed = kPmpHeaderSize;
+  const uint8* itr = data_.get() + kPmpHeaderSize;
+
+  while (strings_.size() < rows_) {
+    const uint8* ptr_to_str_end = static_cast<const uint8*>(

[vandebo (ex-Chrome), 2013/03/28 00:56:04]

variables should generally not be abbreviations and should not include the type,
so string_end, or end_of_string

[tommycli, 2013/03/29 00:16:01]

Done.

+        memchr(itr, '\0', length_ - bytes_parsed));
+
+    // Fail if cannot find null termination. String runs on past file end.
+    if(ptr_to_str_end == NULL) return false;

[vandebo (ex-Chrome), 2013/03/28 00:56:04]

two lines.

[tommycli, 2013/03/29 00:16:01]

Done.

+
+    // Length of string. (+1 to include the termination character).
+    ptrdiff_t len_bytes = ptr_to_str_end - itr + 1;

[vandebo (ex-Chrome), 2013/03/28 00:56:04]

len_bytes = length ?

[tommycli, 2013/03/29 00:16:01]

Done.

+
+    strings_.push_back(reinterpret_cast<const char*>(itr));
+    itr += len_bytes;
+    bytes_parsed += len_bytes;
+  }
+
+  // This means the file has more bytes at the end we haven't parsed.
+  if(bytes_parsed != length_)
+    return false;
+
+  return true;
+}
+
+template<class T>
+bool PmpColumnReader::ValidatePrimitiveArrayLength() {
+  unsigned long calculated_n_rows = (length_ - kPmpHeaderSize) / sizeof(T);

[vandebo (ex-Chrome), 2013/03/28 00:56:04]

Also check that there's no remainder ?

[tommycli, 2013/03/29 00:16:01]

Whoops. Totally forgot about the whole integer division thing.

+
+  return (calculated_n_rows == rows_);
+}
+
+}  // namespace