Add JPEG parser for JPEG decode acceleration

content/common/gpu/media/vaapi_jpeg_parser.cc

+// Copyright 2014 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 "base/logging.h"
+#include "content/common/gpu/media/vaapi_jpeg_parser.h"
+
+using base::BigEndianReader;
+
+namespace content {
+
+VaapiJpegParser::VaapiJpegParser(const uint8_t* buffer, size_t length)
+    : buffer_(buffer), length_(length) {

[Owen Lin, 2014/12/26 07:13:39]

DCHECK on buffer and length ?

[kcwu, 2014/12/27 13:27:34]

Acknowledged.

+}
+
+VaapiJpegParser::~VaapiJpegParser() {
+}
+
+bool VaapiJpegParser::ParseSOF(BigEndianReader reader) {

[Pawel Osciak, 2014/12/26 04:47:40]

Do we really want to copy readers passing them to methods?

[kcwu, 2014/12/27 13:27:34]

I want to make sure the reader of one marker won't read beyond the length of
that marker. So one reader for one marker is necessary (easier).

Regarding to copy, it depends on compiler. I believed modern compilers will do
this kind of simple optimization for us. 

Anyway, I changed it to plain pointer and size.

+  // Spec B.2.2 Frame header syntax
+  uint8_t precision;
+
+  if (!reader.ReadU8(&precision))
+    return false;

[Pawel Osciak, 2014/12/26 04:47:40]

Perhaps you could have a macro
READ_U8_OR_RETURN_FALSE()

[kcwu, 2014/12/27 13:27:34]

Done.

+  if (!reader.ReadU16(&result_.visible_height))
+    return false;
+  if (!reader.ReadU16(&result_.visible_width))
+    return false;
+  if (!reader.ReadU8(&result_.num_component))
+    return false;
+
+  if (precision != 8) {
+    DLOG(ERROR) << "Only support 8 bit precision, not "
+                << static_cast<int>(precision) << " bit";
+    return false;
+  }
+  // Size 64k*64k is the maximum in the JPEG standard. VAAPI doesn't support
+  // larger than 16k*16k.
+  if (result_.visible_height > 16384 || result_.visible_width > 16384) {

[Pawel Osciak, 2014/12/26 04:47:39]

We should not have vaapi specifics in the parser. Please keep it generic.
Vaapi client can check this after parsing and decide to bail out.

[kcwu, 2014/12/27 13:27:34]

Done.

+    DLOG(ERROR) << "VAAPI don't support size(" << result_.visible_width << "*"
+                << result_.visible_height << ") larger than 16k*16k";
+    return false;
+  }
+  if (result_.num_component != 3 ||

[Pawel Osciak, 2014/12/26 04:47:39]

make this a constant?

[kcwu, 2014/12/27 13:27:34]

Done.

+      result_.num_component >= arraysize(result_.components)) {

[Pawel Osciak, 2014/12/26 04:47:40]

What's the max num components by spec? Can we just have max in result?

[kcwu, 2014/12/27 13:27:34]

In theory (generic JPEG spec), it could be 255. But in specific format, the
number is much smaller. For example, JFIF is 1 and 3. In practice, the max is 4
(ref:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjpe...
)
VAAPI only support 3.

+    DLOG(ERROR) << "num_component=" << static_cast<int>(result_.num_component)
+                << " is not supported";
+    return false;
+  }
+
+  for (int i = 0; i < result_.num_component; i++) {

[Pawel Osciak, 2014/12/26 04:47:40]

size_t i

[kcwu, 2014/12/27 13:27:34]

Done.

+    JpegComponent& component = result_.components[i];
+    if (!reader.ReadU8(&component.id))
+      return false;
+    if (component.id > result_.num_component) {
+      DLOG(ERROR) << "component id (" << static_cast<int>(component.id)
+                  << ") should be <= num_component ("
+                  << static_cast<int>(result_.num_component) << ")";
+      return false;
+    }
+    uint8_t hv;
+    if (!reader.ReadU8(&hv))
+      return false;
+    component.horizontal_sampling_factor = hv / 16;

[Pawel Osciak, 2014/12/26 04:47:40]

Do we need to check for more things, like if it's 0, etc?

In general, whenever we read *anything* from the stream, and the spec gives you
clear range of allowed values, we have to verify the value is correct.
See for example in H264Parser IN_RANGE_OR_RETURN().

[kcwu, 2014/12/27 13:27:35]

Done.

+    component.vertical_sampling_factor = hv % 16;
+    if (!reader.ReadU8(&component.quantization_table_selector))
+      return false;
+  }
+
+  if (result_.components[0].horizontal_sampling_factor <
+          result_.components[1].horizontal_sampling_factor ||
+      result_.components[0].horizontal_sampling_factor <
+          result_.components[2].horizontal_sampling_factor) {
+    DLOG(ERROR) << "VAAPI don't supports horizontal sampling factor of Y"

[Pawel Osciak, 2014/12/26 04:47:40]

No vaapi anywhere please.

[kcwu, 2014/12/27 13:27:34]

Done.

+                << " smaller than Cb and Cr";
+    return false;
+  }
+  if (result_.components[0].vertical_sampling_factor <
+          result_.components[1].vertical_sampling_factor ||
+      result_.components[0].vertical_sampling_factor <
+          result_.components[2].vertical_sampling_factor) {
+    DLOG(ERROR) << "VAAPI don't supports vertical sampling factor of Y"
+                << " smaller than Cb and Cr";
+    return false;
+  }
+
+  return true;
+}
+
+bool VaapiJpegParser::ParseDQT(BigEndianReader reader) {
+  // Spec B.2.4.1 Quantization table-specification syntax
+  while (reader.remaining() > 0) {
+    uint8_t tmp;
+    if (!reader.ReadU8(&tmp))
+      return false;
+    uint8_t precision = tmp / 16;
+    uint8_t table_id = tmp % 16;
+    if (precision != 0) {
+      DLOG(ERROR) << "16 bits quantization table is not supported";
+      return false;
+    }
+    if (table_id >= kJpegQuantizationTableNum) {
+      DLOG(ERROR) << "Quantization table id (" << static_cast<int>(table_id)
+                  << ") exceeded " << kJpegQuantizationTableNum;
+      return false;
+    }
+
+    if (!reader.ReadBytes(&result_.q_table[table_id].value,
+                          sizeof(result_.q_table[table_id].value)))
+      return false;
+    result_.q_table[table_id].valid = true;
+  }
+  return true;
+}
+
+bool VaapiJpegParser::ParseDHT(BigEndianReader reader) {
+  // Spec B.2.4.2 Huffman table-specification syntax
+  while (reader.remaining() > 0) {
+    uint8_t tmp;
+    if (!reader.ReadU8(&tmp))
+      return false;
+    int table_class = tmp / 16;
+    int table_id = tmp % 16;
+    if (table_class >= 2) {
+      DLOG(ERROR) << "Invalid table class: " << table_class;
+      return false;
+    }
+    if (table_id >= 2) {
+      DLOG(ERROR) << "Table id(" << table_id << ") >= 2 is invalid for "

[Owen Lin, 2014/12/26 07:13:39]

Is this the result of git cl format ? I thought there are some better format.

[kcwu, 2014/12/27 13:27:34]

Done.

+                                                "baseline profile";
+      return false;
+    }
+
+    JpegHuffmanTable* table;
+    if (table_class == 1)
+      table = &result_.ac_table[table_id];
+    else
+      table = &result_.dc_table[table_id];
+
+    size_t count = 0;
+    if (!reader.ReadBytes(&table->code_length, sizeof(table->code_length)))

[Pawel Osciak, 2014/12/26 04:47:40]

Where does code_length come from? Do we need to validate values in table in
order to not have overflows in loop below?

[kcwu, 2014/12/27 13:27:34]

I don't understand how to overflow. Please clarify?

[Owen Lin, 2014/12/29 09:03:20]

It's actually the length of the |code_length| ... and is
a value decided in compiling time. I believe it should be fine.

+      return false;
+    for (size_t i = 0; i < arraysize(table->code_length); i++)
+      count += table->code_length[i];
+
+    if (count > sizeof(table->code_value)) {
+      DLOG(ERROR) << "Number of code values is invalid: " << count;
+      return false;
+    }
+    if (!reader.ReadBytes(&table->code_value, count))
+      return false;
+    table->valid = true;
+  }
+  return true;
+}
+
+bool VaapiJpegParser::ParseDRI(BigEndianReader reader) {
+  // Spec B.2.4.4 Restart interval definition syntax
+  return reader.ReadU16(&result_.restart_interval) && reader.remaining() == 0;
+}
+
+bool VaapiJpegParser::ParseSOS(BigEndianReader reader) {
+  // Spec B.2.3 Scan header syntax
+  if (!reader.ReadU8(&result_.scan.num_component))
+    return false;
+  if (result_.scan.num_component != result_.num_component) {
+    DLOG(ERROR) << "The number of scan components ("
+                << static_cast<int>(result_.scan.num_component)
+                << ") mismatches the number of image components ("
+                << static_cast<int>(result_.num_component) << ")";
+    return false;
+  }
+
+  for (int i = 0; i < result_.scan.num_component; i++) {
+    JpegScan::Component* component = &result_.scan.components[i];
+    if (!reader.ReadU8(&component->component_selector))
+      return false;
+    uint8_t tmp;
+    if (!reader.ReadU8(&tmp))
+      return false;
+    component->dc_selector = tmp / 16;
+    component->ac_selector = tmp % 16;
+    if (component->component_selector != result_.components[i].id) {
+      DLOG(ERROR) << "component selector mismatches image component id";
+      return false;
+    }
+    if (component->dc_selector >= kJpegHuffmanTableNum_baseline) {
+      DLOG(ERROR) << "DC selector (" << static_cast<int>(component->dc_selector)
+                  << ") should be 0 or 1 for baseline mode";
+      return false;
+    }
+    if (component->ac_selector >= kJpegHuffmanTableNum_baseline) {
+      DLOG(ERROR) << "AC selector (" << static_cast<int>(component->ac_selector)
+                  << ") should be 0 or 1 for baseline mode";
+      return false;
+    }
+  }
+
+  // unused fields, only for value checking

[Pawel Osciak, 2014/12/26 04:47:40]

Capital letter at beginning, dot at end.

[kcwu, 2014/12/27 13:27:34]

Done.

+  uint8_t spectral_selection_start;
+  uint8_t spectral_selection_end;
+  uint8_t point_transform;
+  if (!reader.ReadU8(&spectral_selection_start))
+    return false;
+  if (!reader.ReadU8(&spectral_selection_end))
+    return false;
+  if (!reader.ReadU8(&point_transform))
+    return false;
+  if (spectral_selection_start != 0 || spectral_selection_end != 63) {
+    DLOG(ERROR) << "Spectral selection should be 0,63 for baseline mode";
+    return false;
+  }
+  if (point_transform != 0) {
+    DLOG(ERROR) << "Point transform should be 0 for baseline mode";
+    return false;
+  }
+
+  return true;
+}
+
+bool VaapiJpegParser::ParseSOI(BigEndianReader reader) {
+  // Spec B.2.1 High-level syntax
+  uint8_t marker1;
+  uint8_t marker2;
+  bool has_marker_dqt = false;
+  bool has_marker_sos = false;
+
+  // Once reached SOS, all neccesary data are parsed.
+  while (!has_marker_sos) {
+    if (!reader.ReadU8(&marker1) || marker1 != MARKER1)
+      return false;
+
+    do {
+      if (!reader.ReadU8(&marker2))
+        return false;
+    } while (marker2 == MARKER1);  // skip fill bytes
+
+    uint16_t size;
+    if (!reader.ReadU16(&size))
+      return false;
+    if (reader.remaining() < size) {
+      DLOG(ERROR) << "Ill-formed JPEG. Remaining size (" << reader.remaining()
+                  << ") is smaller than header specified (" << size << ")";
+      return false;
+    }
+
+    // The size includes the size field itself.
+    if (size < sizeof(size)) {
+      DLOG(ERROR) << "Ill-formed JPEG. Segment size (" << size
+                  << ") is smaller than size field (" << sizeof(size) << ")";
+      return false;
+    }
+    size -= sizeof(size);
+
+    switch (marker2) {
+      case SOF0:
+        if (!ParseSOF(BigEndianReader(reader.ptr(), size))) {
+          DLOG(ERROR) << "ParseSOF failed";
+          return false;
+        }
+        break;
+      case DQT:
+        if (!ParseDQT(BigEndianReader(reader.ptr(), size))) {
+          DLOG(ERROR) << "ParseDQT failed";
+          return false;
+        }
+        has_marker_dqt = true;
+        break;
+      case DHT:
+        if (!ParseDHT(BigEndianReader(reader.ptr(), size))) {
+          DLOG(ERROR) << "ParseDHT failed";
+          return false;
+        }
+        break;
+      case DRI:
+        if (!ParseDRI(BigEndianReader(reader.ptr(), size))) {
+          DLOG(ERROR) << "ParseDRI failed";
+          return false;
+        }
+        break;
+      case SOS:
+        if (!ParseSOS(BigEndianReader(reader.ptr(), size))) {
+          DLOG(ERROR) << "ParseSOS failed";
+          return false;
+        }
+        has_marker_sos = true;
+        break;
+      default:
+        DVLOG(4) << "unknown marker " << static_cast<int>(marker2);
+        break;
+    }
+    reader.Skip(size);

[Owen Lin, 2014/12/26 07:13:39]

We should check the return code.

[kcwu, 2014/12/27 13:27:34]

I already check "reader.remaining() < size" above. And if there is really
something wrong, it will also failed when next read. Do you still think we
should check the return value here?

[Owen Lin, 2014/12/29 09:03:20]

Acknowledged.

+  }
+
+  if (!has_marker_dqt) {
+    DLOG(ERROR) << "No DQT marker found";
+    return false;
+  }
+
+  // Scan data follows scan header immediately.
+  result_.scan.data = reader.ptr();
+  result_.scan.data_size = reader.remaining();
+
+  return true;
+}
+
+const JpegParseResult* VaapiJpegParser::Parse() {
+  BigEndianReader reader(reinterpret_cast<const char*>(buffer_), length_);
+  memset(&result_, 0, sizeof(result_));
+
+  uint8_t marker1, marker2;
+  if (!reader.ReadU8(&marker1) || marker1 != MARKER1 ||
+      !reader.ReadU8(&marker2) || marker2 != SOI) {
+    LOG(ERROR) << "Not a JPEG";
+    return NULL;
+  }
+
+  if (ParseSOI(BigEndianReader(reader.ptr(), reader.remaining())))

[Pawel Osciak, 2014/12/26 04:47:40]

Why are we creating a separate reader for each parse method?

[kcwu, 2014/12/27 13:27:34]

Acknowledged.

+    return &result_;
+
+  return NULL;
+}
+
+}  // namespace content