Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
825843002:
Add JPEG decoder for VAAPI JPEG decode acceleration (Closed)
|
Created:
5 years, 12 months ago by kcwu Modified:
5 years, 10 months ago CC:
chromium-reviews, binji+watch_chromium.org, yusukes+watch_chromium.org, tzik, posciak+watch_chromium.org, noelallen1, jam, mcasas+watch_chromium.org, raymes+watch_chromium.org, feature-media-reviews_chromium.org, teravest+watch_chromium.org, darin-cc_chromium.org, nfullagar1, mkwst+moarreviews-renderer_chromium.org, piman+watch_chromium.org, wjia+watch_chromium.org, ihf+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@mjpeg-vaapi-jpeg-parser Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd JPEG decoder for VAAPI JPEG decode acceleration
BUG=335778
TEST=build and run vaapi_jpeg_decoder_unittest
Committed: https://crrev.com/b17ead43a82b5767b86a00ffb59a64e42ed35e6f
Cr-Commit-Position: refs/heads/master@{#313881}
Patch Set 1 #Patch Set 2 : rebase+revise due to parser cl #
Total comments: 85
Patch Set 3 : #
Total comments: 12
Patch Set 4 : #
Total comments: 34
Patch Set 5 : addressed wuchengli's comments #
Total comments: 14
Patch Set 6 : #Patch Set 7 : rebase #
Total comments: 28
Patch Set 8 : addressed posciak's comments #
Total comments: 7
Patch Set 9 : #
Total comments: 9
Patch Set 10 : #Messages
Total messages: 30 (3 generated)
kcwu@chromium.org changed reviewers: + posciak@chromium.org, wuchengli@chromium.org
Please take a look. What I am not sure is adding media::MJPEGPROFILE. Will it make any side effects? p.s. this CL is based on parser CL https://codereview.chromium.org/748023002/ . Please ignore parser part here. I will rebase after the parser submitted.
kcwu@chromium.org changed reviewers: + owenlin@chromium.org
Owen, please help review this one as well. Thanks
rebased parser changes. please take a look.
I haven't finished the review. Please address these first. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... File content/common/gpu/media/va.sigs (right): https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/va.sigs:14: VAStatus vaCreateImage(VADisplay dpy, VAImageFormat *format, int width, int height, VAImage *image); sort https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_decoder.cc (right): https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:89: VaapiWrapper* vaapi_wrapper, VaapiWrapper creation should be done in this class (like VaapiVDA) so the profile won't mismatch. For example, what if the caller passes a vaapi_wrapper that uses H264 profile? https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:104: if (!media::JpegParser::Parse(buffer, buffer_size, &parse_result)) add a DLOG(ERROR) here. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:107: // Create surfaces s/surfaces/a surface/. This only creates one surface. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:113: // Set picture parameter Add periods for all comments. s/parameter/parameters/ https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:134: // Set quantization table Break this function into smaller functions. This function is big. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_decoder.h (right): https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2015 :) https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:5: // This file contains an implementation of a class that provides JPEG decode This should be removed or merged with VaapiJpegDecoder documentation in line 24-35. This doesn't give more information. This file contain the interface, not the implementation. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:19: #include "media/base/limits.h" Move to .cc? https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:20: #include "media/filters/jpeg_parser.h" move to .cc? Please check every #include here. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:24: // TODO(kcwu) comment Remove TODO and add the documentation. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:31: // Clients of this class are expected to pass H264 Annex-B byte stream and update the comments https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:41: VAAPI_ERROR, Document these errors. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:46: typedef base::Callback<void(DecodeStatus error)> ReportErrorToUmaCB; Remove all UMA code because this needs the corresponding histograms.xml changes.. We can add this when we need a histogram. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:49: // |output_pic_cb| notifies the client a surface is to be displayed. remove https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:57: DecodeStatus Decode(const uint8* buffer, Add documentation for this and DecodeDone. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc (right): https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:26: return data_dir.AppendASCII("media").AppendASCII("test").AppendASCII("data"); pixel-1280x720.bin can be removed from this CL since you are using the one in media/test/data/ https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:38: x_display_ = XOpenDisplay(nullptr); How does this work with freon? https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:66: // x_display need to be initialized and possibly freed manually. possibly is vague. Should we free manually or not? https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:86: uint32_t kI420_fourcc = 'I' | '4' << 8 | '2' << 16 | '0' << 24; VA_FOURCC_IYUV? I420 is IYUV. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:111: base::FilePath input_file = GetDataDir().AppendASCII("pixel-1280x720.jpg"); Make this a constant. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:119: EXPECT_EQ("451f28cc874a2206fc5c5a9213b212bd", actual_md5sum); Make this a constant. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:129: // make data corrupted to simulate parse failure Capitalize the first word and add period. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:623: bool VaapiWrapper::GetVaImageWithFormat(VASurfaceID va_surface_id, Is CreateVaImage better? This actually passes format and size. GetVaImageWithFormatAndSize is a bit long. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:635: VA_LOG_ON_ERROR(va_res, "vaCreateImage failed"); VA_SUCCESS_OR_RETURN https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:641: VA_LOG_ON_ERROR(va_res, "vaDeriveImage failed"); s/vaDeriveImage/vaGetImage/ and VA_SUCCESS_OR_RETURN https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:654: return false; It's easier to understand to return true at the end. if (va_res != VA_STATUS_SUCCESS) { vaDestroyImage return false; } return true; https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:47: static scoped_ptr<VaapiWrapper> Create( Can we add another Create for JPEG so we don't need to add a new VideoCodecProfile? https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:115: Add documentation. https://codereview.chromium.org/825843002/diff/20001/content/content_tests.gypi File content/content_tests.gypi (right): https://codereview.chromium.org/825843002/diff/20001/content/content_tests.gy... content/content_tests.gypi:1565: ['chromeos==1 and use_x11 == 1 and target_arch != "arm"', { Please rebase (address the comments in a patchset and rebase in the next). https://codereview.chromium.org/825843002/diff/20001/media/base/video_decoder... File media/base/video_decoder_config.h (right): https://codereview.chromium.org/825843002/diff/20001/media/base/video_decoder... media/base/video_decoder_config.h:67: MJPEGPROFILE = 13, Let's not add a new video profile because vaapi wrapper interfaces uses it. Add a vaapi wrapper function for jpeg.
https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_decoder.cc (right): https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:110: if (!vaapi_wrapper_->CreateSurfaces(size, 1, &va_surfaces)) We should reuse the surface to optimize for MJPEG. Each Jpeg has the same size. Have an Initialize do CreateSurface and move vaapi_wrapper_->DestroySurfaces to destructor?
https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_decoder.cc (right): https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:7: #include <algorithm> not used? https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:8: #include <limits> not used? https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:110: if (!vaapi_wrapper_->CreateSurfaces(size, 1, &va_surfaces)) On 2015/01/12 13:48:04, wuchengli wrote: > We should reuse the surface to optimize for MJPEG. Each Jpeg has the same size. > Have an Initialize do CreateSurface and move vaapi_wrapper_->DestroySurfaces to > destructor? I believe the difficulty is that we need to parse the buffer to know the picture size. So, maybe it would be nice if we parse the the buffer outside the class. class VaapiJpegDecoder { VaapiJpegDecoder(const gfx::Size& size); DecodeStatus Decode(const media::JpegParseResult parse_result, VASurfaceID output); VASurfaceID CreateSurface(gfx::Size size); DestroySurface(VASurfaceID id); } https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_decoder.h (right): https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:15: #include "base/memory/linked_ptr.h" not used ? https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:16: #include "base/memory/ref_counted.h" not used? https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:66: scoped_refptr<VASurface> available_va_surface_; Not used? https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc (right): https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:45: ASSERT_TRUE(wrapper_.get()); Do we need the get()? https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:72: std::string& md5sum) { Why not just pass the expected md5sum and rename it to VerifyDecode ? https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:105: wrapper_->ReturnVaImageForTesting(&image); Why there is no DecodeDone() ? Is this because it would be freed with the wrapper ? https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:642: if (va_res != VA_STATUS_SUCCESS) free the resource of image (since it has been created) ? https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:654: return false; On 2015/01/12 09:08:53, wuchengli wrote: > It's easier to understand to return true at the end. > > if (va_res != VA_STATUS_SUCCESS) { > vaDestroyImage > return false; > } > return true; +1 https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:118: gfx::Size size, const gfx::Size&
https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... File content/common/gpu/media/va.sigs (right): https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/va.sigs:14: VAStatus vaCreateImage(VADisplay dpy, VAImageFormat *format, int width, int height, VAImage *image); On 2015/01/12 09:08:51, wuchengli wrote: > sort Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_decoder.cc (right): https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:7: #include <algorithm> On 2015/01/13 06:17:58, Owen Lin wrote: > not used? Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:8: #include <limits> On 2015/01/13 06:17:58, Owen Lin wrote: > not used? Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:89: VaapiWrapper* vaapi_wrapper, On 2015/01/12 09:08:51, wuchengli wrote: > VaapiWrapper creation should be done in this class (like VaapiVDA) so the > profile won't mismatch. For example, what if the caller passes a vaapi_wrapper > that uses H264 profile? This class is modeled after VaapiH264Decoder which create VaapiWrapper outside. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:104: if (!media::JpegParser::Parse(buffer, buffer_size, &parse_result)) On 2015/01/12 09:08:52, wuchengli wrote: > add a DLOG(ERROR) here. Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:107: // Create surfaces On 2015/01/12 09:08:51, wuchengli wrote: > s/surfaces/a surface/. This only creates one surface. Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:110: if (!vaapi_wrapper_->CreateSurfaces(size, 1, &va_surfaces)) On 2015/01/13 06:17:58, Owen Lin wrote: > On 2015/01/12 13:48:04, wuchengli wrote: > > We should reuse the surface to optimize for MJPEG. Each Jpeg has the same > size. > > Have an Initialize do CreateSurface and move vaapi_wrapper_->DestroySurfaces > to > > destructor? > > I believe the difficulty is that we need to parse the buffer to know the picture > size. > So, maybe it would be nice if we parse the the buffer outside the class. > > class VaapiJpegDecoder { > VaapiJpegDecoder(const gfx::Size& size); > DecodeStatus Decode(const media::JpegParseResult parse_result, VASurfaceID > output); > VASurfaceID CreateSurface(gfx::Size size); > DestroySurface(VASurfaceID id); > } > Done. I moved the responsibility of surface creation to the client. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:113: // Set picture parameter On 2015/01/12 09:08:52, wuchengli wrote: > Add periods for all comments. s/parameter/parameters/ Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:134: // Set quantization table On 2015/01/12 09:08:52, wuchengli wrote: > Break this function into smaller functions. This function is big. Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_decoder.h (right): https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/12 09:08:52, wuchengli wrote: > 2015 :) Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:5: // This file contains an implementation of a class that provides JPEG decode On 2015/01/12 09:08:52, wuchengli wrote: > This should be removed or merged with VaapiJpegDecoder documentation in line > 24-35. This doesn't give more information. > > This file contain the interface, not the implementation. Done This is modified from vaapi_h264_decoder.h ;) https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:15: #include "base/memory/linked_ptr.h" On 2015/01/13 06:17:58, Owen Lin wrote: > not used ? Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:16: #include "base/memory/ref_counted.h" On 2015/01/13 06:17:58, Owen Lin wrote: > not used? Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:19: #include "media/base/limits.h" On 2015/01/12 09:08:52, wuchengli wrote: > Move to .cc? Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:20: #include "media/filters/jpeg_parser.h" On 2015/01/12 09:08:52, wuchengli wrote: > move to .cc? Please check every #include here. Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:24: // TODO(kcwu) comment On 2015/01/12 09:08:52, wuchengli wrote: > Remove TODO and add the documentation. Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:31: // Clients of this class are expected to pass H264 Annex-B byte stream and On 2015/01/12 09:08:52, wuchengli wrote: > update the comments Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:41: VAAPI_ERROR, On 2015/01/12 09:08:52, wuchengli wrote: > Document these errors. Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:46: typedef base::Callback<void(DecodeStatus error)> ReportErrorToUmaCB; On 2015/01/12 09:08:52, wuchengli wrote: > Remove all UMA code because this needs the corresponding histograms.xml > changes.. We can add this when we need a histogram. Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:49: // |output_pic_cb| notifies the client a surface is to be displayed. On 2015/01/12 09:08:52, wuchengli wrote: > remove Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:57: DecodeStatus Decode(const uint8* buffer, On 2015/01/12 09:08:52, wuchengli wrote: > Add documentation for this and DecodeDone. Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:66: scoped_refptr<VASurface> available_va_surface_; On 2015/01/13 06:17:58, Owen Lin wrote: > Not used? Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc (right): https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:26: return data_dir.AppendASCII("media").AppendASCII("test").AppendASCII("data"); On 2015/01/12 09:08:52, wuchengli wrote: > pixel-1280x720.bin can be removed from this CL since you are using the one in > media/test/data/ Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:38: x_display_ = XOpenDisplay(nullptr); On 2015/01/12 09:08:52, wuchengli wrote: > How does this work with freon? Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:45: ASSERT_TRUE(wrapper_.get()); On 2015/01/13 06:17:58, Owen Lin wrote: > Do we need the get()? Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:66: // x_display need to be initialized and possibly freed manually. On 2015/01/12 09:08:52, wuchengli wrote: > possibly is vague. Should we free manually or not? Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:72: std::string& md5sum) { On 2015/01/13 06:17:58, Owen Lin wrote: > Why not just pass the expected md5sum and rename it to VerifyDecode ? Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:86: uint32_t kI420_fourcc = 'I' | '4' << 8 | '2' << 16 | '0' << 24; On 2015/01/12 09:08:53, wuchengli wrote: > VA_FOURCC_IYUV? I420 is IYUV. No. vaCreateImage failed with VA_FOURCC_IYUV https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:105: wrapper_->ReturnVaImageForTesting(&image); On 2015/01/13 06:17:58, Owen Lin wrote: > Why there is no DecodeDone() ? Is this because it would be freed with the > wrapper ? Yes. I added DecodeDone anyway. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:111: base::FilePath input_file = GetDataDir().AppendASCII("pixel-1280x720.jpg"); On 2015/01/12 09:08:52, wuchengli wrote: > Make this a constant. Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:119: EXPECT_EQ("451f28cc874a2206fc5c5a9213b212bd", actual_md5sum); On 2015/01/12 09:08:53, wuchengli wrote: > Make this a constant. Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:129: // make data corrupted to simulate parse failure On 2015/01/12 09:08:53, wuchengli wrote: > Capitalize the first word and add period. Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:623: bool VaapiWrapper::GetVaImageWithFormat(VASurfaceID va_surface_id, On 2015/01/12 09:08:53, wuchengli wrote: > Is CreateVaImage better? This actually passes format and size. > GetVaImageWithFormatAndSize is a bit long. Done. In order to emphasis its difference to GetVaImageForTesting, I renamed GetVaImageForTesting to GetDerivedVaImageForTesting. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:635: VA_LOG_ON_ERROR(va_res, "vaCreateImage failed"); On 2015/01/12 09:08:53, wuchengli wrote: > VA_SUCCESS_OR_RETURN Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:641: VA_LOG_ON_ERROR(va_res, "vaDeriveImage failed"); On 2015/01/12 09:08:53, wuchengli wrote: > s/vaDeriveImage/vaGetImage/ and VA_SUCCESS_OR_RETURN Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:642: if (va_res != VA_STATUS_SUCCESS) On 2015/01/13 06:17:59, Owen Lin wrote: > free the resource of image (since it has been created) ? Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:654: return false; On 2015/01/12 09:08:53, wuchengli wrote: > It's easier to understand to return true at the end. > > if (va_res != VA_STATUS_SUCCESS) { > vaDestroyImage > return false; > } > return true; Since I need to call vaDestroyImage for failed case, I revised the code and tried not to duplicate. PTAL. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:115: On 2015/01/12 09:08:53, wuchengli wrote: > Add documentation. Done. https://codereview.chromium.org/825843002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:118: gfx::Size size, On 2015/01/13 06:17:59, Owen Lin wrote: > const gfx::Size& Done. https://codereview.chromium.org/825843002/diff/20001/content/content_tests.gypi File content/content_tests.gypi (right): https://codereview.chromium.org/825843002/diff/20001/content/content_tests.gy... content/content_tests.gypi:1565: ['chromeos==1 and use_x11 == 1 and target_arch != "arm"', { On 2015/01/12 09:08:53, wuchengli wrote: > Please rebase (address the comments in a patchset and rebase in the next). Done. I rebased anyway because some parts depend on new code. But are there anything wrong with this line? https://codereview.chromium.org/825843002/diff/20001/media/base/video_decoder... File media/base/video_decoder_config.h (right): https://codereview.chromium.org/825843002/diff/20001/media/base/video_decoder... media/base/video_decoder_config.h:67: MJPEGPROFILE = 13, On 2015/01/12 09:08:53, wuchengli wrote: > Let's not add a new video profile because vaapi wrapper interfaces uses it. Add > a vaapi wrapper function for jpeg. Done.
lgtm % nits https://codereview.chromium.org/825843002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_decoder.cc (right): https://codereview.chromium.org/825843002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:7: #include <vector> not used. https://codereview.chromium.org/825843002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:91: : vaapi_wrapper_(vaapi_wrapper) { DCHECK on vaapi_wrapper ? https://codereview.chromium.org/825843002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_decoder.h (right): https://codereview.chromium.org/825843002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:30: // |vaapi_wrapper| should be initialized. Can you explain more on what the wrapper you would be expected? https://codereview.chromium.org/825843002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:36: // cooresponding VA-API methods according to parsed JPEG result s/cooresponding/corresponding/ https://codereview.chromium.org/825843002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:37: // |parse_result|. Decoded data will be output to given |va_surface|. be outputted to the given ? https://codereview.chromium.org/825843002/diff/40001/content/content_tests.gypi File content/content_tests.gypi (right): https://codereview.chromium.org/825843002/diff/40001/content/content_tests.gy... content/content_tests.gypi:1662: '../build/linux/system.gyp:x11', do we still need x11 now ?
https://codereview.chromium.org/825843002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_decoder.cc (right): https://codereview.chromium.org/825843002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:7: #include <vector> On 2015/01/19 09:29:31, Owen Lin wrote: > not used. Done. https://codereview.chromium.org/825843002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:91: : vaapi_wrapper_(vaapi_wrapper) { On 2015/01/19 09:29:31, Owen Lin wrote: > DCHECK on vaapi_wrapper ? Done. https://codereview.chromium.org/825843002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_decoder.h (right): https://codereview.chromium.org/825843002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:30: // |vaapi_wrapper| should be initialized. On 2015/01/19 09:29:31, Owen Lin wrote: > Can you explain more on what the wrapper you would be expected? Done. https://codereview.chromium.org/825843002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:36: // cooresponding VA-API methods according to parsed JPEG result On 2015/01/19 09:29:31, Owen Lin wrote: > s/cooresponding/corresponding/ Done. https://codereview.chromium.org/825843002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:37: // |parse_result|. Decoded data will be output to given |va_surface|. On 2015/01/19 09:29:31, Owen Lin wrote: > be outputted to the given ? Done. https://codereview.chromium.org/825843002/diff/40001/content/content_tests.gypi File content/content_tests.gypi (right): https://codereview.chromium.org/825843002/diff/40001/content/content_tests.gy... content/content_tests.gypi:1662: '../build/linux/system.gyp:x11', On 2015/01/19 09:29:31, Owen Lin wrote: > do we still need x11 now ? Done.
https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_decoder.cc (right): https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:13: static JpegHuffmanTable default_dc_table[kJpegMaxHuffmanTableNumBaseline] = { Why this is exposed to media namespace? If no special reason, put this in anonymous namespace. Same for default_ac_table. https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:29: static JpegHuffmanTable default_ac_table[kJpegMaxHuffmanTableNumBaseline] = { s/default_ac_table/kDefaultAcTable/ https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:100: if (jpeg.frame_header.visible_height > kMaxDimension || nit: check visible_width first then visible_height. Be consistent with the rest of this file. https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:102: DLOG(ERROR) << "VAAPI don't support size(" s/don't/doesn't/. Same below. https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_decoder.h (right): https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:20: // acceleration into the JDA framework. s/JDA/JpegDecodeAccelerator/ It's an uncommon acronym. https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:23: // VaapiWrapper, parse JPEG picture via ParseJpegPicture, and then pass them to s/ParseJpegPicture/media::ParseJpegPicture/ https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc (right): https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:45: ASSERT_TRUE(wrapper_); This should be CHECK. ASSERT_TRUE doesn't fail the test. The test will fail later when |wrapper_| is dereferenced. https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:84: format.bits_per_pixel = 12; We can use this. for (size_t i = 0; i < media::VideoFrame::NumPlanes(I420); ++i) bits_per_pixel += media::VideoFrame::PlaneBitsPerPixel(I420, i); https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:94: size.width() * size.height() * 2); use media::AllocationSize. https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:122: << "failed to read input data from " << input_file.value(); input_file and data creation can be done in SetUp. https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:133: EXPECT_FALSE(VerifyDecode(parse_result, "")); This should call wrapper_->CreateSurfaces and decoder_->Decode dircetly. We want to make sure Decode returns false, not other functions. https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:184: It's cleaner to break ProfileToVAProfile to two functions -- line 106-112 in ProfileToVAProfile and others to a new function. ProfileToVAProfile is called here. The new function is called in Initialize. VaInitialize and GetSupportedVaProfiles can be moved back to Initialize. CreateForVideoCodec can just call Create. How do you think? https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:736: VA_LOG_ON_ERROR(va_res, "vaDestroyImage failed"); If vaMapBuffer fails and vaDestroyImage succeeds, this function will incorrectly return success. It's clearer just return false here and reutrn true in line 739. https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:143: bool GetVaImage(VASurfaceID va_surface_id, GetDerivedVaImageForTesting, GetVaImage, and ReturnVaImage are used for testing in this CL. All of them should have ForTesting suffix or all shouldn't. Please choose one. If GetVaImage or ReturnVaImage are used by the jpeg accelerator in the next CL, they should be renamed in the next CL. https://codereview.chromium.org/825843002/diff/60001/content/content_tests.gypi File content/content_tests.gypi (right): https://codereview.chromium.org/825843002/diff/60001/content/content_tests.gy... content/content_tests.gypi:1667: 'common/gpu/media/vaapi_jpeg_decoder.cc', This should be moved to content/content_common.gypi. https://codereview.chromium.org/825843002/diff/60001/content/content_tests.gy... content/content_tests.gypi:1668: 'common/gpu/media/vaapi_jpeg_decoder_unittest.cc', GN should also have this.
https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_decoder.h (right): https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:41: bool Decode(const media::JpegParseResult& parse_result, This can be static and pass VaapiWrapper here. It's much more clear VaapiJpegDecoder is stateless and doesn't own |vaapi_wrapper|. Please also explain the prerequisite of |va_surface| here. For example, the surface should be created with the picture size (or >= the picture size?).
https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_decoder.cc (right): https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:13: static JpegHuffmanTable default_dc_table[kJpegMaxHuffmanTableNumBaseline] = { On 2015/01/20 07:55:53, wuchengli wrote: > Why this is exposed to media namespace? If no special reason, put this in > anonymous namespace. Same for default_ac_table. Done. Actually they are not exposed to media due to "static". https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:29: static JpegHuffmanTable default_ac_table[kJpegMaxHuffmanTableNumBaseline] = { On 2015/01/20 07:55:53, wuchengli wrote: > s/default_ac_table/kDefaultAcTable/ Done. https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:100: if (jpeg.frame_header.visible_height > kMaxDimension || On 2015/01/20 07:55:53, wuchengli wrote: > nit: check visible_width first then visible_height. Be consistent with the rest > of this file. Done. https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:102: DLOG(ERROR) << "VAAPI don't support size(" On 2015/01/20 07:55:53, wuchengli wrote: > s/don't/doesn't/. Same below. Done. https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_decoder.h (right): https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:20: // acceleration into the JDA framework. On 2015/01/20 07:55:53, wuchengli wrote: > s/JDA/JpegDecodeAccelerator/ It's an uncommon acronym. Done. https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:23: // VaapiWrapper, parse JPEG picture via ParseJpegPicture, and then pass them to On 2015/01/20 07:55:53, wuchengli wrote: > s/ParseJpegPicture/media::ParseJpegPicture/ Done. https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:41: bool Decode(const media::JpegParseResult& parse_result, On 2015/01/20 15:41:41, wuchengli wrote: > This can be static and pass VaapiWrapper here. It's much more clear > VaapiJpegDecoder is stateless and doesn't own |vaapi_wrapper|. Please also > explain the prerequisite of |va_surface| here. For example, the surface should > be created with the picture size (or >= the picture size?). Done. https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc (right): https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:45: ASSERT_TRUE(wrapper_); On 2015/01/20 07:55:53, wuchengli wrote: > This should be CHECK. ASSERT_TRUE doesn't fail the test. The test will fail > later when |wrapper_| is dereferenced. No. CHECK will crash. ASSERT_* will fail the test and EXPECT_* don't fail the test. https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:84: format.bits_per_pixel = 12; On 2015/01/20 07:55:53, wuchengli wrote: > We can use this. > for (size_t i = 0; i < media::VideoFrame::NumPlanes(I420); ++i) > bits_per_pixel += media::VideoFrame::PlaneBitsPerPixel(I420, i); I'm wondering should we make it complicated here. And note this is only unittest. https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:94: size.width() * size.height() * 2); On 2015/01/20 07:55:53, wuchengli wrote: > use media::AllocationSize. Done. https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:122: << "failed to read input data from " << input_file.value(); On 2015/01/20 07:55:53, wuchengli wrote: > input_file and data creation can be done in SetUp. Done. https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:133: EXPECT_FALSE(VerifyDecode(parse_result, "")); On 2015/01/20 07:55:53, wuchengli wrote: > This should call wrapper_->CreateSurfaces and decoder_->Decode dircetly. We want > to make sure Decode returns false, not other functions. Done. https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:184: On 2015/01/20 07:55:53, wuchengli wrote: > It's cleaner to break ProfileToVAProfile to two functions -- line 106-112 in > ProfileToVAProfile and others to a new function. ProfileToVAProfile is called > here. The new function is called in Initialize. > > VaInitialize and GetSupportedVaProfiles can be moved back to Initialize. > CreateForVideoCodec can just call Create. How do you think? Done. Good idea. https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:736: VA_LOG_ON_ERROR(va_res, "vaDestroyImage failed"); On 2015/01/20 07:55:53, wuchengli wrote: > If vaMapBuffer fails and vaDestroyImage succeeds, this function will incorrectly > return success. It's clearer just return false here and reutrn true in line 739. Done. https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/825843002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:143: bool GetVaImage(VASurfaceID va_surface_id, On 2015/01/20 07:55:54, wuchengli wrote: > GetDerivedVaImageForTesting, GetVaImage, and ReturnVaImage are used for testing > in this CL. All of them should have ForTesting suffix or all shouldn't. Please > choose one. If GetVaImage or ReturnVaImage are used by the jpeg accelerator in > the next CL, they should be renamed in the next CL. Done. https://codereview.chromium.org/825843002/diff/60001/content/content_tests.gypi File content/content_tests.gypi (right): https://codereview.chromium.org/825843002/diff/60001/content/content_tests.gy... content/content_tests.gypi:1667: 'common/gpu/media/vaapi_jpeg_decoder.cc', On 2015/01/20 07:55:54, wuchengli wrote: > This should be moved to content/content_common.gypi. Done. https://codereview.chromium.org/825843002/diff/60001/content/content_tests.gy... content/content_tests.gypi:1668: 'common/gpu/media/vaapi_jpeg_decoder_unittest.cc', On 2015/01/20 07:55:54, wuchengli wrote: > GN should also have this. This followed vaapi_h264_decoder_unittest, which is not in GN as well.
LGTM. Please still fix my comments in this patchset. Pawel. Need owner approval. Thanks. https://codereview.chromium.org/825843002/diff/80001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_decoder.cc (right): https://codereview.chromium.org/825843002/diff/80001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:114: DLOG(ERROR) << "VAAPI don't supports horizontal sampling factor of Y" nit: doesn't https://codereview.chromium.org/825843002/diff/80001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:123: DLOG(ERROR) << "VAAPI don't supports vertical sampling factor of Y" nit: doesn't https://codereview.chromium.org/825843002/diff/80001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_decoder.h (right): https://codereview.chromium.org/825843002/diff/80001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:27: // any other thread. This can be removed. A static method doesn't have states and can be called by two places at the same time. I don't think we need to emphasis a static method does not use another thread. https://codereview.chromium.org/825843002/diff/80001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:42: DISALLOW_COPY_AND_ASSIGN(VaapiJpegDecoder); DISALLOW_IMPLICIT_CONSTRUCTORS. Let's also disallow the default constructor. https://codereview.chromium.org/825843002/diff/80001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc (right): https://codereview.chromium.org/825843002/diff/80001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:81: const uint32_t kI420_fourcc = VA_FOURCC('I', '4', '2', '0'); s/kI420_fourcc/kI420Fourcc/ https://codereview.chromium.org/825843002/diff/80001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/825843002/diff/80001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:355: return nullptr; false https://codereview.chromium.org/825843002/diff/80001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:358: return nullptr; false
https://codereview.chromium.org/825843002/diff/80001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_decoder.cc (right): https://codereview.chromium.org/825843002/diff/80001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:114: DLOG(ERROR) << "VAAPI don't supports horizontal sampling factor of Y" On 2015/01/21 13:19:58, wuchengli wrote: > nit: doesn't Done. https://codereview.chromium.org/825843002/diff/80001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.cc:123: DLOG(ERROR) << "VAAPI don't supports vertical sampling factor of Y" On 2015/01/21 13:19:58, wuchengli wrote: > nit: doesn't Done. https://codereview.chromium.org/825843002/diff/80001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_decoder.h (right): https://codereview.chromium.org/825843002/diff/80001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:27: // any other thread. On 2015/01/21 13:19:58, wuchengli wrote: > This can be removed. A static method doesn't have states and can be called by > two places at the same time. I don't think we need to emphasis a static method > does not use another thread. Done. https://codereview.chromium.org/825843002/diff/80001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder.h:42: DISALLOW_COPY_AND_ASSIGN(VaapiJpegDecoder); On 2015/01/21 13:19:58, wuchengli wrote: > DISALLOW_IMPLICIT_CONSTRUCTORS. Let's also disallow the default constructor. Done. https://codereview.chromium.org/825843002/diff/80001/content/common/gpu/media... File content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc (right): https://codereview.chromium.org/825843002/diff/80001/content/common/gpu/media... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:81: const uint32_t kI420_fourcc = VA_FOURCC('I', '4', '2', '0'); On 2015/01/21 13:19:58, wuchengli wrote: > s/kI420_fourcc/kI420Fourcc/ Done. https://codereview.chromium.org/825843002/diff/80001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/825843002/diff/80001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:355: return nullptr; On 2015/01/21 13:19:58, wuchengli wrote: > false Done. https://codereview.chromium.org/825843002/diff/80001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:358: return nullptr; On 2015/01/21 13:19:58, wuchengli wrote: > false Done.
https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... File content/common/gpu/media/vaapi_jpeg_decoder.cc (right): https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... content/common/gpu/media/vaapi_jpeg_decoder.cc:90: static bool IsVaapiSupportedJpeg(const media::JpegParseResult& jpeg) { Please add a short doc. https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... content/common/gpu/media/vaapi_jpeg_decoder.cc:92: // larger than 16k*16k. s/larger/resolutions larger/ https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... content/common/gpu/media/vaapi_jpeg_decoder.cc:160: for (int j = 0; j < 64; j++) Could we use arraysize instead of 64? Could we also have: const JpegQuantizationTable* q_table = q_table[i]; unsigned char* quantiser_table = iq_matrix->quantiser_table[i]; compile_assert(arraysize(q_table) == arraysize(quantiser_table)...)? https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... content/common/gpu/media/vaapi_jpeg_decoder.cc:234: (max_h_factor * 8); Do we need to check these are not 0? https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... File content/common/gpu/media/vaapi_jpeg_decoder.h (right): https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... content/common/gpu/media/vaapi_jpeg_decoder.h:33: // |va_surface| should be created with size at least as the picture size. at least as large https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... File content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc (right): https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:31: return data_dir.AppendASCII("media").AppendASCII("test").AppendASCII("data"); media::GetTestDataFilePath() should work here. https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... File content/common/gpu/media/vaapi_wrapper.cc (right): https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... content/common/gpu/media/vaapi_wrapper.cc:118: static VAProfile FallbackVaProfileIfNecessary( We should not fall back to use Constrained if VAProfile Baseline is passed directly to Create(). The fallback mechanism is needed because of a problem with media::VideoCodecProfile and not with VAProfile. We fall back, because VideoCodecProfile Baseline usually means VAProfile Constrained. But VAProfile Baseline will never mean VAProfile Constrained. So we can only fall back when passed VideoCodecProfile Baseline, not on VAProfile Baseline as well. With that we should not need this split into two methods. https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... File content/common/gpu/media/vaapi_wrapper.h (right): https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:48: // Create VaapiWrapper. ...for VAProfile. https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:56: // Create VaapiWrapper for video codec. It maps VideoCodecProfile |profile| for VideoCodecProfile https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:134: // for testing only. Last sentence not applicable anymore? https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:141: bool GetVaImage(VASurfaceID va_surface_id, This is confusing, why would I ever prefer using this over GetDerivedVaImage(), if GetDerivedVaImage() can (seemingly?) infer format and size? https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:148: // GetVaImage(). ... or GetDeriveVaImage() ?
https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... File content/common/gpu/media/vaapi_jpeg_decoder.cc (right): https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... content/common/gpu/media/vaapi_jpeg_decoder.cc:90: static bool IsVaapiSupportedJpeg(const media::JpegParseResult& jpeg) { On 2015/01/25 08:51:11, Pawel Osciak wrote: > Please add a short doc. Done. https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... content/common/gpu/media/vaapi_jpeg_decoder.cc:92: // larger than 16k*16k. On 2015/01/25 08:51:11, Pawel Osciak wrote: > s/larger/resolutions larger/ Done. https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... content/common/gpu/media/vaapi_jpeg_decoder.cc:160: for (int j = 0; j < 64; j++) On 2015/01/25 08:51:11, Pawel Osciak wrote: > Could we use arraysize instead of 64? > Could we also have: > const JpegQuantizationTable* q_table = q_table[i]; > unsigned char* quantiser_table = iq_matrix->quantiser_table[i]; > compile_assert(arraysize(q_table) == arraysize(quantiser_table)...)? Done. https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... content/common/gpu/media/vaapi_jpeg_decoder.cc:234: (max_h_factor * 8); On 2015/01/25 08:51:11, Pawel Osciak wrote: > Do we need to check these are not 0? Done. Check 0 in IsVaapiSupportedJpeg and DCHECK here. https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... File content/common/gpu/media/vaapi_jpeg_decoder.h (right): https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... content/common/gpu/media/vaapi_jpeg_decoder.h:33: // |va_surface| should be created with size at least as the picture size. On 2015/01/25 08:51:11, Pawel Osciak wrote: > at least as large Done. https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... File content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc (right): https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... content/common/gpu/media/vaapi_jpeg_decoder_unittest.cc:31: return data_dir.AppendASCII("media").AppendASCII("test").AppendASCII("data"); On 2015/01/25 08:51:11, Pawel Osciak wrote: > media::GetTestDataFilePath() should work here. Done. https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... File content/common/gpu/media/vaapi_wrapper.cc (right): https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... content/common/gpu/media/vaapi_wrapper.cc:118: static VAProfile FallbackVaProfileIfNecessary( On 2015/01/25 08:51:11, Pawel Osciak wrote: > We should not fall back to use Constrained if VAProfile Baseline is passed > directly to Create(). > > The fallback mechanism is needed because of a problem with > media::VideoCodecProfile and not with VAProfile. We fall back, because > VideoCodecProfile Baseline usually means VAProfile Constrained. But VAProfile > Baseline will never mean VAProfile Constrained. So we can only fall back when > passed VideoCodecProfile Baseline, not on VAProfile Baseline as well. With that > we should not need this split into two methods. The problem is GetSupportedVaProfiles() cannot be called before VaIntialize(). If we don't split this, we may need to split Initalize() as my previous patch. Now I use a flag to control fallback or not. How do you think? https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... File content/common/gpu/media/vaapi_wrapper.h (right): https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:48: // Create VaapiWrapper. On 2015/01/25 08:51:11, Pawel Osciak wrote: > ...for VAProfile. Done. https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:56: // Create VaapiWrapper for video codec. It maps VideoCodecProfile |profile| On 2015/01/25 08:51:11, Pawel Osciak wrote: > for VideoCodecProfile Done. https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:134: // for testing only. On 2015/01/25 08:51:11, Pawel Osciak wrote: > Last sentence not applicable anymore? Done. https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:141: bool GetVaImage(VASurfaceID va_surface_id, On 2015/01/25 08:51:11, Pawel Osciak wrote: > This is confusing, why would I ever prefer using this over GetDerivedVaImage(), > if GetDerivedVaImage() can (seemingly?) infer format and size? Done. Add comments about this in GetDerivedVaImage. https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:148: // GetVaImage(). On 2015/01/25 08:51:11, Pawel Osciak wrote: > ... or GetDeriveVaImage() ? Done.
https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... File content/common/gpu/media/vaapi_jpeg_decoder.cc (right): https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... content/common/gpu/media/vaapi_jpeg_decoder.cc:234: (max_h_factor * 8); On 2015/01/26 12:12:09, kcwu wrote: > On 2015/01/25 08:51:11, Pawel Osciak wrote: > > Do we need to check these are not 0? > > Done. > Check 0 in IsVaapiSupportedJpeg and DCHECK here. Are we sure checking width/height is enough to guarantee mcu_cols/mcu_rows is not 0? Also, would CHECK be better than DCHECK? https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... File content/common/gpu/media/vaapi_wrapper.cc (right): https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... content/common/gpu/media/vaapi_wrapper.cc:118: static VAProfile FallbackVaProfileIfNecessary( On 2015/01/26 12:12:09, kcwu wrote: > On 2015/01/25 08:51:11, Pawel Osciak wrote: > > We should not fall back to use Constrained if VAProfile Baseline is passed > > directly to Create(). > > > > The fallback mechanism is needed because of a problem with > > media::VideoCodecProfile and not with VAProfile. We fall back, because > > VideoCodecProfile Baseline usually means VAProfile Constrained. But VAProfile > > Baseline will never mean VAProfile Constrained. So we can only fall back when > > passed VideoCodecProfile Baseline, not on VAProfile Baseline as well. With > that > > we should not need this split into two methods. > > The problem is GetSupportedVaProfiles() cannot be called before VaIntialize(). > If we don't split this, we may need to split Initalize() as my previous patch. > > Now I use a flag to control fallback or not. How do you think? Can we do the fallback in ProfileToVAProfile only? https://chromiumcodereview.appspot.com/825843002/diff/140001/content/common/g... File content/common/gpu/media/vaapi_jpeg_decoder.cc (right): https://chromiumcodereview.appspot.com/825843002/diff/140001/content/common/g... content/common/gpu/media/vaapi_jpeg_decoder.cc:94: jpeg.frame_header.visible_height <= 1) { This also fails if height or width is 1, but error message says it should be at least 1, so 1 should be a success? https://chromiumcodereview.appspot.com/825843002/diff/140001/content/common/g... content/common/gpu/media/vaapi_jpeg_decoder.cc:172: "number of quantization entries mismatched"); s/number/sizes/ https://chromiumcodereview.appspot.com/825843002/diff/140001/content/common/g... File content/common/gpu/media/vaapi_wrapper.h (right): https://chromiumcodereview.appspot.com/825843002/diff/140001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:133: // |size| are different to surface internal representation. The VAImage s/to/from/ What will happen is they are different? Will we do color conversion/scaling? https://chromiumcodereview.appspot.com/825843002/diff/140001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:187: bool va_profile_degraded, Please document what this is.
https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... File content/common/gpu/media/vaapi_wrapper.cc (right): https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... content/common/gpu/media/vaapi_wrapper.cc:118: static VAProfile FallbackVaProfileIfNecessary( On 2015/01/27 00:55:44, Pawel Osciak wrote: > On 2015/01/26 12:12:09, kcwu wrote: > > On 2015/01/25 08:51:11, Pawel Osciak wrote: > > > We should not fall back to use Constrained if VAProfile Baseline is passed > > > directly to Create(). > > > > > > The fallback mechanism is needed because of a problem with > > > media::VideoCodecProfile and not with VAProfile. We fall back, because > > > VideoCodecProfile Baseline usually means VAProfile Constrained. But > VAProfile > > > Baseline will never mean VAProfile Constrained. So we can only fall back > when > > > passed VideoCodecProfile Baseline, not on VAProfile Baseline as well. With > > that > > > we should not need this split into two methods. > > > > The problem is GetSupportedVaProfiles() cannot be called before VaIntialize(). > > If we don't split this, we may need to split Initalize() as my previous patch. > > > > Now I use a flag to control fallback or not. How do you think? > > Can we do the fallback in ProfileToVAProfile only? Do you mean like what I did in patch set 4? https://chromiumcodereview.appspot.com/825843002/diff/60001/content/common/gp...
https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... File content/common/gpu/media/vaapi_jpeg_decoder.cc (right): https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... content/common/gpu/media/vaapi_jpeg_decoder.cc:234: (max_h_factor * 8); On 2015/01/27 00:55:44, Pawel Osciak wrote: > On 2015/01/26 12:12:09, kcwu wrote: > > On 2015/01/25 08:51:11, Pawel Osciak wrote: > > > Do we need to check these are not 0? > > > > Done. > > Check 0 in IsVaapiSupportedJpeg and DCHECK here. > > Are we sure checking width/height is enough to guarantee mcu_cols/mcu_rows is > not 0? Also, would CHECK be better than DCHECK? I handled overflow in the new patch. It could be guarantee not 0 and DCHECK is enough. https://chromiumcodereview.appspot.com/825843002/diff/140001/content/common/g... File content/common/gpu/media/vaapi_jpeg_decoder.cc (right): https://chromiumcodereview.appspot.com/825843002/diff/140001/content/common/g... content/common/gpu/media/vaapi_jpeg_decoder.cc:94: jpeg.frame_header.visible_height <= 1) { On 2015/01/27 00:55:45, Pawel Osciak wrote: > This also fails if height or width is 1, but error message says it should be at > least 1, so 1 should be a success? Done. https://chromiumcodereview.appspot.com/825843002/diff/140001/content/common/g... content/common/gpu/media/vaapi_jpeg_decoder.cc:172: "number of quantization entries mismatched"); On 2015/01/27 00:55:45, Pawel Osciak wrote: > s/number/sizes/ "sizes" lead me to think sizeof(q_table[i].value[0]) instead. https://chromiumcodereview.appspot.com/825843002/diff/140001/content/common/g... File content/common/gpu/media/vaapi_wrapper.h (right): https://chromiumcodereview.appspot.com/825843002/diff/140001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:133: // |size| are different to surface internal representation. The VAImage On 2015/01/27 00:55:45, Pawel Osciak wrote: > s/to/from/ > > What will happen is they are different? Will we do color conversion/scaling? Done.
On 2015/01/27 08:12:26, kcwu wrote: > https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... > File content/common/gpu/media/vaapi_jpeg_decoder.cc (right): > > https://chromiumcodereview.appspot.com/825843002/diff/120001/content/common/g... > content/common/gpu/media/vaapi_jpeg_decoder.cc:234: (max_h_factor * 8); > On 2015/01/27 00:55:44, Pawel Osciak wrote: > > On 2015/01/26 12:12:09, kcwu wrote: > > > On 2015/01/25 08:51:11, Pawel Osciak wrote: > > > > Do we need to check these are not 0? > > > > > > Done. > > > Check 0 in IsVaapiSupportedJpeg and DCHECK here. > > > > Are we sure checking width/height is enough to guarantee mcu_cols/mcu_rows is > > not 0? Also, would CHECK be better than DCHECK? > > I handled overflow in the new patch. > It could be guarantee not 0 and DCHECK is enough. > > https://chromiumcodereview.appspot.com/825843002/diff/140001/content/common/g... > File content/common/gpu/media/vaapi_jpeg_decoder.cc (right): > > https://chromiumcodereview.appspot.com/825843002/diff/140001/content/common/g... > content/common/gpu/media/vaapi_jpeg_decoder.cc:94: > jpeg.frame_header.visible_height <= 1) { > On 2015/01/27 00:55:45, Pawel Osciak wrote: > > This also fails if height or width is 1, but error message says it should be > at > > least 1, so 1 should be a success? > > Done. > > https://chromiumcodereview.appspot.com/825843002/diff/140001/content/common/g... > content/common/gpu/media/vaapi_jpeg_decoder.cc:172: "number of quantization > entries mismatched"); > On 2015/01/27 00:55:45, Pawel Osciak wrote: > > s/number/sizes/ > > "sizes" lead me to think sizeof(q_table[i].value[0]) instead. > > https://chromiumcodereview.appspot.com/825843002/diff/140001/content/common/g... > File content/common/gpu/media/vaapi_wrapper.h (right): > > https://chromiumcodereview.appspot.com/825843002/diff/140001/content/common/g... > content/common/gpu/media/vaapi_wrapper.h:133: // |size| are different to surface > internal representation. The VAImage > On 2015/01/27 00:55:45, Pawel Osciak wrote: > > s/to/from/ > > > > What will happen is they are different? Will we do color conversion/scaling? > > Done. Pawel. PTAL.
https://codereview.chromium.org/825843002/diff/160001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/825843002/diff/160001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:365: const base::Closure& report_error_to_uma_cb) { Not used. Remove. https://codereview.chromium.org/825843002/diff/160001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/825843002/diff/160001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:57: // |profile| to VAProfile according to supported profiles. I'm not sure what "according to supported profiles" tells us. Remove? https://codereview.chromium.org/825843002/diff/160001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:140: // |mem|. If |format| doesn't equal to the internal format, it will do format s/it will do format.../the underlying implementation will do format conversion if supported/. I think it's clearer. https://codereview.chromium.org/825843002/diff/160001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:142: // smaller or equal to the surface. If |size| is smaller, the image will be s/smaller/smaller than/
https://codereview.chromium.org/825843002/diff/160001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/825843002/diff/160001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:142: // smaller or equal to the surface. If |size| is smaller, the image will be On 2015/01/30 06:38:59, wuchengli wrote: > s/smaller/smaller than/ To be clear, I'm referring to the first "smaller".
https://codereview.chromium.org/825843002/diff/160001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/825843002/diff/160001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:365: const base::Closure& report_error_to_uma_cb) { On 2015/01/30 06:38:59, wuchengli wrote: > Not used. Remove. Done. https://codereview.chromium.org/825843002/diff/160001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/825843002/diff/160001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:57: // |profile| to VAProfile according to supported profiles. On 2015/01/30 06:38:59, wuchengli wrote: > I'm not sure what "according to supported profiles" tells us. Remove? Done. https://codereview.chromium.org/825843002/diff/160001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:140: // |mem|. If |format| doesn't equal to the internal format, it will do format On 2015/01/30 06:38:59, wuchengli wrote: > s/it will do format.../the underlying implementation will do format conversion > if supported/. I think it's clearer. Done. https://codereview.chromium.org/825843002/diff/160001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:142: // smaller or equal to the surface. If |size| is smaller, the image will be On 2015/01/30 06:39:48, wuchengli wrote: > On 2015/01/30 06:38:59, wuchengli wrote: > > s/smaller/smaller than/ > To be clear, I'm referring to the first "smaller". Done.
lgtm
The CQ bit was checked by kcwu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/825843002/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/b17ead43a82b5767b86a00ffb59a64e42ed35e6f Cr-Commit-Position: refs/heads/master@{#313881} |
