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()
+    : data_(NULL),
+      length_(0),
+      rows_(0),
+      strings_(),
+      field_type_(kPmpFieldTypeInvalid) { }
+
+PmpColumnReader::~PmpColumnReader() { }
+
+bool PmpColumnReader::InitFromFile(const base::FilePath& filepath,
+                                   uint32* rows_read) {
+  base::MemoryMappedFile file;

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

So using a memory mapped file means you need enough virtual address space to map
the entire file.  But you really don't need a memory mapped file.  How about
using GetFileSize() and ReadFile()

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

Add a DCHECK that we're on the FILE thread?

[tommycli, 2013/04/01 22:19:18]

Done.

[tommycli, 2013/04/01 22:19:18]

Done.

+
+  if(!file.Initialize(filepath) ||

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

nit: s/if(/if (/g

[tommycli, 2013/04/01 22:19:18]

Done.

+     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_GT(length_, kPmpHeaderSize + row);
+
+  if(field_type_ == kPmpFieldTypeString && row < rows_) {

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

We like early exits of functions, especially when it keeps the indent down...
so:
if (field_type_ != kPmpFieldTypeString || row >= rows_)
  return false;
...

[tommycli, 2013/04/01 22:19:18]

Nice. That's definitely an improvement.

+    *result = strings_[row];
+    return true;
+  }
+
+  return false;
+}
+
+bool PmpColumnReader::ReadUInt32(const uint32 row, uint32* result) const {
+  DCHECK(data_.get() != NULL);
+  DCHECK_GT(length_, kPmpHeaderSize + row*sizeof(uint32));

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

nit: space around math operators: row * sizeof()

[tommycli, 2013/04/01 22:19:18]

Done.

+
+  if(field_type_ == kPmpFieldTypeUInt32 && row < rows_) {
+    *result = reinterpret_cast<uint32*>(data_.get() + kPmpHeaderSize)[row];
+    return true;
+  }
+
+  return false;
+}
+
+bool PmpColumnReader::ReadDouble64(const uint32 row, double* result) const {
+  DCHECK(data_.get() != NULL);
+  DCHECK_GT(length_, kPmpHeaderSize + row*sizeof(double));
+
+  if(field_type_ == kPmpFieldTypeDouble64 && row < rows_) {
+    *result = reinterpret_cast<double*>(data_.get() + kPmpHeaderSize)[row];
+    return true;
+  }
+
+  return false;
+}
+
+bool PmpColumnReader::ReadUInt8(const uint32 row, uint8* result) const {
+  DCHECK(data_.get() != NULL);
+  DCHECK_GT(length_, kPmpHeaderSize + row*sizeof(uint8));
+
+  if(field_type_ == kPmpFieldTypeUInt8 && row < rows_) {
+    *result = reinterpret_cast<uint8*>(data_.get() + kPmpHeaderSize)[row];
+    return true;
+  }
+
+  return false;
+}
+
+bool PmpColumnReader::ReadUInt64(const uint32 row, uint64* result) const {
+  DCHECK(data_.get() != NULL);
+  DCHECK_GT(length_, kPmpHeaderSize + row*sizeof(uint64));
+
+  if(field_type_ == kPmpFieldTypeUInt64 && row < rows_) {
+    *result = reinterpret_cast<uint64*>(data_.get() + kPmpHeaderSize)[row];
+    return true;
+  }
+
+  return false;
+}
+
+bool PmpColumnReader::ParseData(uint32* rows_read) {
+  DCHECK(data_.get() != NULL);
+  DCHECK_GE(length_, kPmpHeaderSize);
+
+  // Check all magic bytes.
+  if (memcmp(&kPmpMagic1, &data_[kPmpMagic1Offset], sizeof(kPmpMagic1)) != 0 ||
+      memcmp(&kPmpMagic2, &data_[kPmpMagic2Offset], sizeof(kPmpMagic2)) != 0 ||
+      memcmp(&kPmpMagic3, &data_[kPmpMagic3Offset], sizeof(kPmpMagic3)) != 0 ||
+      memcmp(&kPmpMagic4, &data_[kPmpMagic4Offset], sizeof(kPmpMagic4)) != 0) {
+    return false;
+  }
+
+  // Read the field type

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

nit: remove redundant comment.

[tommycli, 2013/04/01 22:19:18]

Done.

+  field_type_ = *(reinterpret_cast<uint16*>(&data_[kPmpFieldType1Offset]));
+
+  // Verify if field type matches second declaration
+  if (field_type_ != *(reinterpret_cast<uint16*>(&data_[kPmpFieldType2Offset])))
+    return false;
+
+  // Read the number of rows expected.

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

nit: redundant comment.

[tommycli, 2013/04/01 22:19:18]

Done.

+  rows_ = *(reinterpret_cast<uint32*>(&data_[kPmpRowCountOffset]));
+
+  bool parse_success = false;
+

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

nit: extra line.

[tommycli, 2013/04/01 22:19:18]

Done.

+  switch (field_type_) {
+    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:
+      return false;
+      break;
+  }
+
+  if(parse_success) {

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

if (parse_success && rows_read)
  *rows_read = rows_;
return parse_success;

[tommycli, 2013/04/01 22:19:18]

That's ninja.

+    // Set the number of rows read. Though it's not valid unless we return true.
+    if(rows_read != NULL)
+      *rows_read = rows_;
+    return true;
+  }
+
+  return false;
+}
+
+bool PmpColumnReader::IndexStrings() {
+  DCHECK(data_.get() != NULL);
+  DCHECK_GE(length_, kPmpHeaderSize);
+
+  strings_.clear();

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

not needed

[tommycli, 2013/04/01 22:19:18]

Done.

+  strings_.reserve(rows_);
+
+  size_t bytes_parsed = kPmpHeaderSize;
+  const uint8* itr = data_.get() + kPmpHeaderSize;

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

nit: We usually use it for iterators... though this isn't actually an iterator. 
Maybe data?

[tommycli, 2013/04/01 22:19:18]

Done.

+
+  while (strings_.size() < rows_) {
+    const uint8* string_end = static_cast<const uint8*>(
+        memchr(itr, '\0', length_ - bytes_parsed));
+
+    // Fail if cannot find null termination. String runs on past file end.
+    if(string_end == NULL)
+      return false;
+
+    // Length of string. (+1 to include the termination character).
+    ptrdiff_t length_in_bytes = string_end - itr + 1;

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

Any word back from Picasa folks on the string encoding used?

[tommycli, 2013/04/01 22:19:18]

I fwded you email from them.

[vandebo (ex-Chrome), 2013/04/02 18:10:55]

You got an non-answer back.  Can you peek into the code, or test picasa with
some non english strings to see what's going on?

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

Done.

+
+    strings_.push_back(reinterpret_cast<const char*>(itr));
+    itr += length_in_bytes;
+    bytes_parsed += length_in_bytes;
+  }
+
+  // This means the file has more bytes at the end we haven't parsed.

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

redundant comment.

[tommycli, 2013/04/01 22:19:18]

Done.

+  if(bytes_parsed != length_)
+    return false;

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

return bytes_parsed == length_;

[tommycli, 2013/04/01 22:19:18]

Done.

+
+  return true;
+}
+
+template<class T>
+bool PmpColumnReader::ValidatePrimitiveArrayLength() {

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

I think you can inline this template without much loss.

[tommycli, 2013/04/01 22:19:18]

Done.

+  size_t body_length = length_ - kPmpHeaderSize;
+  size_t expected_body_length = rows_ * sizeof(T);
+
+  return body_length == expected_body_length;
+}
+
+}  // namespace