Save both original frame and output frame into files if the absolute differences is larger than the…

Save both original frame and output frame into files if the absolute differences is larger than the threshold

Add VideoFrameQualityValidator::SaveFrameToFile().
It converts frame format from yuv to rgb and then saves frames into png files.

BUG=

[hsiaochingc, 2015-08-12 06:08:33]

hsiaochingc@chromium.org changed reviewers:
+ owenlin@chromium.org, wuchengli@chromium.org

[Owen Lin, 2015-08-12 08:07:48]

https://codereview.chromium.org/1286983002/diff/1/content/common/gpu/media/vi...
File content/common/gpu/media/video_encode_accelerator_unittest.cc (right):

https://codereview.chromium.org/1286983002/diff/1/content/common/gpu/media/vi...
content/common/gpu/media/video_encode_accelerator_unittest.cc:714:
CHECK(difference < kDecodeSimilarityThreshold)
I thought we will crash at CHECK and then no output will be generated.

https://codereview.chromium.org/1286983002/diff/1/content/common/gpu/media/vi...
content/common/gpu/media/video_encode_accelerator_unittest.cc:727:
base::FilePath filename) {
const base::FilePath &

https://codereview.chromium.org/1286983002/diff/1/content/common/gpu/media/vi...
content/common/gpu/media/video_encode_accelerator_unittest.cc:728: size_t
row_bytes = frame->coded_size().width() * 4u;
use visible size for rgb_bytes.

https://codereview.chromium.org/1286983002/diff/1/content/common/gpu/media/vi...
content/common/gpu/media/video_encode_accelerator_unittest.cc:729: uint8*
rgb_pixels = reinterpret_cast<uint8*>(
How do you free the rgb_pixels

https://codereview.chromium.org/1286983002/diff/1/content/common/gpu/media/vi...
content/common/gpu/media/video_encode_accelerator_unittest.cc:746: if (success)
{
CHECK(gfx::PNGCodec::...);

[wuchengli, 2015-09-04 01:44:15]

Please rebase and address the review comments.

[hsiaochingc, 2015-09-04 03:00:22]

Patchset #2 (id:20001) has been deleted

[hsiaochingc, 2015-09-04 03:01:49]

PTAL

[hsiaochingc, 2015-09-04 03:22:44]

Patchset #2 (id:40001) has been deleted

[Owen Lin, 2015-09-04 03:34:47]

https://codereview.chromium.org/1286983002/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/video_encode_accelerator_unittest.cc (right):

https://codereview.chromium.org/1286983002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/video_encode_accelerator_unittest.cc:783:
SaveFrameToFile(output_frame, base::FilePath::FromUTF8Unsafe(filename));
Remove EXPECT_TRUE at l.773
Add the following line here. 
ADD_FAILURE() << "difference = " ......

https://codereview.chromium.org/1286983002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/video_encode_accelerator_unittest.cc:790: size_t
row_bytes = frame->visible_rect().width() * 4u;
const int RGB32_BYTES_PER_PIXEL = 4;

https://codereview.chromium.org/1286983002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/video_encode_accelerator_unittest.cc:804:
CHECK(gfx::PNGCodec::Encode(
LOG_ASSERT

https://codereview.chromium.org/1286983002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/video_encode_accelerator_unittest.cc:808:
base::WriteFile(filename, reinterpret_cast<char*>(&*png_output.begin()),
&png_output[0]

[Owen Lin, 2015-09-04 07:03:56]

lgtm % one nit

https://chromiumcodereview.appspot.com/1286983002/diff/80001/content/common/g...
File content/common/gpu/media/video_encode_accelerator_unittest.cc (right):

https://chromiumcodereview.appspot.com/1286983002/diff/80001/content/common/g...
content/common/gpu/media/video_encode_accelerator_unittest.cc:810:
base::AlignedFree(rgb_pixels);
move to the line above WriteFile.

[hsiaochingc, 2015-09-04 07:11:29]

The CQ bit was checked by hsiaochingc@chromium.org

[hsiaochingc, 2015-09-04 07:11:29]

The patchset sent to the CQ was uploaded after l-g-t-m from owenlin@chromium.org
Link to the patchset: https://codereview.chromium.org/1286983002/#ps100001
(title: "Address review comments")

[commit-bot: I haz the power, 2015-09-04 07:11:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1286983002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1286983002/100001

[commit-bot: I haz the power, 2015-09-04 07:20:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-04 07:20:19]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Owen Lin, 2015-09-04 08:05:33]

On 2015/09/04 07:20:19, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

You need to wait lgtm from Wucheng or Pawel.

[wuchengli, 2015-09-04 11:59:44]

https://codereview.chromium.org/1286983002/diff/100001/content/common/gpu/med...
File content/common/gpu/media/video_encode_accelerator_unittest.cc (right):

https://codereview.chromium.org/1286983002/diff/100001/content/common/gpu/med...
content/common/gpu/media/video_encode_accelerator_unittest.cc:1: // Copyright
2013 The Chromium Authors. All rights reserved.
IIRC, the first line of the change description will be git subject. Please use a
shorter subject (<70chars).

Add BUG number.

Add TESTS=

https://codereview.chromium.org/1286983002/diff/100001/content/common/gpu/med...
content/common/gpu/media/video_encode_accelerator_unittest.cc:636: 
Document if the id starts from 0. It's confusing line 654 initializes it to 0
and the first id will be 1. Let's start at 0 because input_id_ also starts at 0.

https://codereview.chromium.org/1286983002/diff/100001/content/common/gpu/med...
content/common/gpu/media/video_encode_accelerator_unittest.cc:773: // Save both
origin and output frames to files if its difference is larger
s/its/their/

https://codereview.chromium.org/1286983002/diff/100001/content/common/gpu/med...
content/common/gpu/media/video_encode_accelerator_unittest.cc:774: // than
kDecodeSimilarityThreshold
add . at the end.

https://codereview.chromium.org/1286983002/diff/100001/content/common/gpu/med...
content/common/gpu/media/video_encode_accelerator_unittest.cc:792:
base::AlignedAlloc(row_bytes * frame->coded_size().height() +
Why do we need the alignment?

https://codereview.chromium.org/1286983002/diff/100001/content/common/gpu/med...
content/common/gpu/media/video_encode_accelerator_unittest.cc:793:
media::VideoFrame::kFrameSizePadding,
Is this required?

https://codereview.chromium.org/1286983002/diff/100001/content/common/gpu/med...
content/common/gpu/media/video_encode_accelerator_unittest.cc:801:
frame->stride(media::VideoFrame::kUPlane), row_bytes, media::YV12);
Is the original frame also YV12? We should document it.

s/kUPlane/kUVPlane/

https://codereview.chromium.org/1286983002/diff/100001/content/common/gpu/med...
content/common/gpu/media/video_encode_accelerator_unittest.cc:805: rgb_pixels,
gfx::PNGCodec::FORMAT_RGBA, frame->coded_size(),
why this is not visible_rect()?

https://codereview.chromium.org/1286983002/diff/100001/content/common/gpu/med...
content/common/gpu/media/video_encode_accelerator_unittest.cc:811: 
remove blank line

https://codereview.chromium.org/1286983002/diff/100001/content/content_tests....
File content/content_tests.gypi (right):

https://codereview.chromium.org/1286983002/diff/100001/content/content_tests....
content/content_tests.gypi:1763: '../third_party/ffmpeg/ffmpeg.gyp:ffmpeg',
Why do we need this? for which include?

[wuchengli, 2016-01-05 14:52:39]

wuchengli@chromium.org changed reviewers:
- wuchengli@chromium.org