Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
27498002:
Add vaapi_h264_decoder_test. (Closed)
|
Created:
7 years, 2 months ago by chihchung Modified:
6 years, 10 months ago CC:
chromium-reviews, jam, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, wuchengli Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd vaapi_h264_decoder_unittest program.
The program reads an H.264 bitstream file and writes a file containing
the decoded frames in I420 format. Alternatively it verifies MD5 sum
of the decoded frames. It can be used for development verification and
regression testing.
BUG=none
TEST=run through H.264.1 Base/Ext/Main profile test streams on link.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250987
Patch Set 1 #Patch Set 2 : fix destruct order #
Total comments: 51
Patch Set 3 : address review comments #
Total comments: 8
Patch Set 4 : address review comments #Patch Set 5 : change to gtest and add md5 verification #
Total comments: 35
Patch Set 6 : address review comments #
Total comments: 2
Patch Set 7 : split into GetVaImage/ReturnVaImage #
Total comments: 4
Patch Set 8 : Add ForTesting suffix #
Total comments: 24
Patch Set 9 : address review comments #
Total comments: 4
Patch Set 10 : address review comments #Patch Set 11 : rebase #Patch Set 12 : fix error found by clang #Patch Set 13 : fix more clang errors #Patch Set 14 : Make it compile for component=shared_library #Patch Set 15 : Add gfx_geometry dependency #
Messages
Total messages: 37 (0 generated)
Initial pass. Please read and apply: http://www.chromium.org/developers/coding-style http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml You should also find "git cl format" useful. Why is the test not in gtest? To be honest I'm not exactly thrilled with the additional functions in the vaapi wrapper, but if they are to stay, you should use VideoFrames, not raw buffers. Since there are a lot of comments, I'll do a second major review after you address the current comments. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... File content/common/gpu/media/vaapi_h264_decoder_test.cc (right): https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. Why is this test not using gtest (even though you include it in dependencies)? It should. How is it supposed to be run? https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:5: #include <stdio.h> Headers should be in lexicographical order. But see comments about stdio usage below. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:18: class VaapiH264DecoderLoop { Please comment on what the class does. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:31: scoped_ptr<VaapiH264Decoder> decoder_; Methods should precede member variables. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:37: // Members need to be freed manually. Which members? https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:39: FILE* output_file_; // output data is written to this file Please use file_util::AppendToFile() instead of stdio.h methods. See example in content/common/gpu/media/video_encode_accelerator_unittest.cc. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:48: // Free one surface and put it on available_surfaces_ list. This may be called Does this really free? It looks like it just recycles it. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:49: // from another thread. Which thread? What is the threading model of this class? https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:50: void RecycleSurface(VASurfaceID va_surface_id); You could use VASurface class and pass to it RecycleSurface() as the destruction callback. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:61: VaapiH264DecoderLoop::VaapiH264DecoderLoop() : x_display_(NULL), New line and 4-space indent on multiline initializer lists. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:90: media::VideoCodecProfile profile = media::H264PROFILE_HIGH; Shouldn't profile be a parameter of the test, depending on the file? https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:92: base::Unretained(this), Indent. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:110: decoder_->SetStream((uint8*)(data_.c_str()), data_.size(), 0 /* input_id */); No c-style casts, here and everywhere. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:133: case VaapiH264Decoder::kRanOutOfStreamData: Brace on this line. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:151: void *buffer; star goes next to void. Here and everywhere else. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:163: void VaapiH264DecoderLoop::RecycleSurface(VASurfaceID va_surface_id) { As mentioned above, you can use VASurface and pass this as the destruction callback. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:182: // TODO: make sure all surfaces are returned Todos have to be in this format: TODO(name): https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:196: int main(int argc, char** argv) { Please use gtest. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_wrapper.cc:425: *size = image->width * image->height * 3 / 2; There are some useful functions in VideoFrame for calculating sizes, addresses of planes, etc. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_wrapper.cc:467: goto out; Don't use goto. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_wrapper.h:83: // released by calling free(). The function returns true if successful. Can you instead use VideoFrame class for this?
Thanks a lot for reviewing! Please take another look. Thanks. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... File content/common/gpu/media/vaapi_h264_decoder_test.cc (right): https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2013/10/20 23:53:11, posciak wrote: > Why is this test not using gtest (even though you include it in dependencies)? > It should. > How is it supposed to be run? It is supposed to be run as a standalone program, not a unit test. The gtest dependency is removed. Comments added: // This program is run like this: // DISPLAY=:0 ./vaapi_h264_decoder_test -v=1 input.264 output.yuv // The input is read from input.264 and output written to output.yuv https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:5: #include <stdio.h> On 2013/10/20 23:53:11, posciak wrote: > Headers should be in lexicographical order. But see comments about stdio usage > below. Yes, I did that originally, but then presubmit check told me C system files should be before C++ system files :) https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:18: class VaapiH264DecoderLoop { On 2013/10/20 23:53:11, posciak wrote: > Please comment on what the class does. Done. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:31: scoped_ptr<VaapiH264Decoder> decoder_; On 2013/10/20 23:53:11, posciak wrote: > Methods should precede member variables. Done. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:37: // Members need to be freed manually. On 2013/10/20 23:53:11, posciak wrote: > Which members? Done. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:39: FILE* output_file_; // output data is written to this file On 2013/10/20 23:53:11, posciak wrote: > Please use file_util::AppendToFile() instead of stdio.h methods. See example in > content/common/gpu/media/video_encode_accelerator_unittest.cc. Is there some advantage using that over fwrite? (Originally I used AppendToFile(), but then I found I need to open an empty file before appending to it, so I use the current form.) https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:48: // Free one surface and put it on available_surfaces_ list. This may be called On 2013/10/20 23:53:11, posciak wrote: > Does this really free? It looks like it just recycles it. Yes, this just recycles it. Fixed. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:49: // from another thread. On 2013/10/20 23:53:11, posciak wrote: > Which thread? What is the threading model of this class? I misunderstood the threading model of VaapiH264Decoder, now this program is all single-thread and all lock stuff are removed. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:50: void RecycleSurface(VASurfaceID va_surface_id); On 2013/10/20 23:53:11, posciak wrote: > You could use VASurface class and pass to it RecycleSurface() as the destruction > callback. I think I did that in RefillSurfaces(), or I should do it some other way? https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:61: VaapiH264DecoderLoop::VaapiH264DecoderLoop() : x_display_(NULL), On 2013/10/20 23:53:11, posciak wrote: > New line and 4-space indent on multiline initializer lists. Done. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:90: media::VideoCodecProfile profile = media::H264PROFILE_HIGH; On 2013/10/20 23:53:11, posciak wrote: > Shouldn't profile be a parameter of the test, depending on the file? Yes, I just picked a value which looks like a larger subset of H.264 to avoid the trouble parsing the stream, and it seems to work fine for base/main profile streams also. I don't really understand this parameter in VA API. Maybe VA API can use this parameter to reserved less resources if given profile needs less, or fails immediately if some profile is not supported. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:92: base::Unretained(this), On 2013/10/20 23:53:11, posciak wrote: > Indent. Done. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:110: decoder_->SetStream((uint8*)(data_.c_str()), data_.size(), 0 /* input_id */); On 2013/10/20 23:53:11, posciak wrote: > No c-style casts, here and everywhere. Done. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:133: case VaapiH264Decoder::kRanOutOfStreamData: On 2013/10/20 23:53:11, posciak wrote: > Brace on this line. Done. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:151: void *buffer; On 2013/10/20 23:53:11, posciak wrote: > star goes next to void. Here and everywhere else. Done. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:163: void VaapiH264DecoderLoop::RecycleSurface(VASurfaceID va_surface_id) { On 2013/10/20 23:53:11, posciak wrote: > As mentioned above, you can use VASurface and pass this as the destruction > callback. I did, or I should do it some other way? https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:182: // TODO: make sure all surfaces are returned On 2013/10/20 23:53:11, posciak wrote: > Todos have to be in this format: > > TODO(name): Done. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:196: int main(int argc, char** argv) { On 2013/10/20 23:53:11, posciak wrote: > Please use gtest. As explained above, this is not a unit test (although I'm thinking about building unit tests using this program), so I think we don't need gtest. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_wrapper.cc:425: *size = image->width * image->height * 3 / 2; On 2013/10/20 23:53:11, posciak wrote: > There are some useful functions in VideoFrame for calculating sizes, addresses > of planes, etc. Done. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_wrapper.cc:467: goto out; On 2013/10/20 23:53:11, posciak wrote: > Don't use goto. Done. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_wrapper.h:83: // released by calling free(). The function returns true if successful. On 2013/10/20 23:53:11, posciak wrote: > Can you instead use VideoFrame class for this? Done.
What do you think about my gtest suggestion? What is your plan to do with this if it's committed like this? https://chromiumcodereview.appspot.com/27498002/diff/4001/content/common/gpu/... File content/common/gpu/media/vaapi_h264_decoder_test.cc (right): https://chromiumcodereview.appspot.com/27498002/diff/4001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder_test.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2013/10/21 09:33:05, chihchung wrote: > On 2013/10/20 23:53:11, posciak wrote: > > Why is this test not using gtest (even though you include it in dependencies)? > > It should. > > How is it supposed to be run? > > It is supposed to be run as a standalone program, not a unit test. The gtest > dependency is removed. > > Comments added: > // This program is run like this: > // DISPLAY=:0 ./vaapi_h264_decoder_test -v=1 input.264 output.yuv > // The input is read from input.264 and output written to output.yuv But ultimately this is to be used for tests, frame comparisons for yuv output from decoder against reference yuv streams. You will have to write a gtest anyway. I think the right thing to do here is to do what vdatest does, turn this into a gtest, have it accept input and output and have short input/output streams committed to testdata dir and otherwise use external streams for real tests. https://chromiumcodereview.appspot.com/27498002/diff/4001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder_test.cc:5: #include <stdio.h> On 2013/10/21 09:33:05, chihchung wrote: > On 2013/10/20 23:53:11, posciak wrote: > > Headers should be in lexicographical order. But see comments about stdio usage > > below. > > Yes, I did that originally, but then presubmit check told me C system files > should be before C++ system files :) Perhaps if you used <cstdio> instead of <stdio.h>, as you should've. https://chromiumcodereview.appspot.com/27498002/diff/4001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder_test.cc:39: FILE* output_file_; // output data is written to this file On 2013/10/21 09:33:05, chihchung wrote: > On 2013/10/20 23:53:11, posciak wrote: > > Please use file_util::AppendToFile() instead of stdio.h methods. See example > in > > content/common/gpu/media/video_encode_accelerator_unittest.cc. > > Is there some advantage using that over fwrite? > > (Originally I used AppendToFile(), but then I found I need to open an empty file > before appending to it, so I use the current form.) Yes, that was one of the reasons I pointed you to the example. Please use AppendToFile, even if just to be consistent and more readable. But stuff like not having to have code to manage/fopen/free FILE* pointer is good enough reason. https://chromiumcodereview.appspot.com/27498002/diff/4001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder_test.cc:50: void RecycleSurface(VASurfaceID va_surface_id); On 2013/10/21 09:33:05, chihchung wrote: > On 2013/10/20 23:53:11, posciak wrote: > > You could use VASurface class and pass to it RecycleSurface() as the > destruction > > callback. > > I think I did that in RefillSurfaces(), or I should do it some other way? Use VASurface class in c/c/gpu/media/va_surface.h, which does exactly this already, in ReleaseCB callback. https://chromiumcodereview.appspot.com/27498002/diff/4001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder_test.cc:90: media::VideoCodecProfile profile = media::H264PROFILE_HIGH; On 2013/10/21 09:33:05, chihchung wrote: > On 2013/10/20 23:53:11, posciak wrote: > > Shouldn't profile be a parameter of the test, depending on the file? > > Yes, I just picked a value which looks like a larger subset of H.264 to avoid > the trouble parsing the stream, and it seems to work fine for base/main profile > streams also. > Right, but did you test the other way around? In Chrome we select the profile, so for base we use base, for main we use main. If you use high for everything it may work well, but you may not catch a problem with using base for base for example. > I don't really understand this parameter in VA API. Maybe VA API can use this > parameter to reserved less resources if given profile needs less, or fails > immediately if some profile is not supported. Most probably. Digging in libva-intel-driver would probably give you an answer. https://chromiumcodereview.appspot.com/27498002/diff/38001/content/common/gpu... File content/common/gpu/media/vaapi_h264_decoder_test.cc (right): https://chromiumcodereview.appspot.com/27498002/diff/38001/content/common/gpu... content/common/gpu/media/vaapi_h264_decoder_test.cc:65: // These members (x_display_, output_file_, picture_count_, num_surfaces_) picture_count_ and num_surfaces are not really freed. And you don't really zero them out either, apart from on construction. https://chromiumcodereview.appspot.com/27498002/diff/38001/content/common/gpu... content/common/gpu/media/vaapi_h264_decoder_test.cc:69: int picture_count_; // number of pictures already outputted s/picture_count_/num_outputted_pictures_ https://chromiumcodereview.appspot.com/27498002/diff/38001/content/common/gpu... File content/common/gpu/media/vaapi_wrapper.cc (right): https://chromiumcodereview.appspot.com/27498002/diff/38001/content/common/gpu... content/common/gpu/media/vaapi_wrapper.cc:420: void* mem) { What do you think of using VideoFrame::WrapExternalYuvData, saving to disk and then releasing this, without copying? You don't return them immediately anyway. Not a big deal though. https://chromiumcodereview.appspot.com/27498002/diff/38001/content/common/gpu... File content/common/gpu/media/vaapi_wrapper.h (right): https://chromiumcodereview.appspot.com/27498002/diff/38001/content/common/gpu... content/common/gpu/media/vaapi_wrapper.h:85: scoped_refptr<media::VideoFrame> GetI420FromSurface( Rename to VideoFrameFromVASurface or something like that.
Thanks a lot for the review. Most of the issues are fixed. I will work on the unit test part tomorrow. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... File content/common/gpu/media/vaapi_h264_decoder_test.cc (right): https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:5: #include <stdio.h> On 2013/11/21 06:31:41, posciak wrote: > On 2013/10/21 09:33:05, chihchung wrote: > > On 2013/10/20 23:53:11, posciak wrote: > > > Headers should be in lexicographical order. But see comments about stdio > usage > > > below. > > > > Yes, I did that originally, but then presubmit check told me C system files > > should be before C++ system files :) > > Perhaps if you used <cstdio> instead of <stdio.h>, as you should've. It's not needed anymore, so removed. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:39: FILE* output_file_; // output data is written to this file On 2013/11/21 06:31:41, posciak wrote: > On 2013/10/21 09:33:05, chihchung wrote: > > On 2013/10/20 23:53:11, posciak wrote: > > > Please use file_util::AppendToFile() instead of stdio.h methods. See example > > in > > > content/common/gpu/media/video_encode_accelerator_unittest.cc. > > > > Is there some advantage using that over fwrite? > > > > (Originally I used AppendToFile(), but then I found I need to open an empty > file > > before appending to it, so I use the current form.) > > Yes, that was one of the reasons I pointed you to the example. Please use > AppendToFile, even if just to be consistent and more readable. > But stuff like not having to have code to manage/fopen/free FILE* pointer is > good enough reason. Good idea. Done. https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:50: void RecycleSurface(VASurfaceID va_surface_id); On 2013/11/21 06:31:41, posciak wrote: > On 2013/10/21 09:33:05, chihchung wrote: > > On 2013/10/20 23:53:11, posciak wrote: > > > You could use VASurface class and pass to it RecycleSurface() as the > > destruction > > > callback. > > > > I think I did that in RefillSurfaces(), or I should do it some other way? > > Use VASurface class in c/c/gpu/media/va_surface.h, which does exactly this > already, in ReleaseCB callback. Sorry I still don't understand. This is the code in RefillSurfaces(): VASurface::ReleaseCB release_cb = base::Bind( &VaapiH264DecoderLoop::RecycleSurface, base::Unretained(this)); scoped_refptr<VASurface> surface( new VASurface(available_surfaces_[i], release_cb)); It is using VASurface and ReleaseCB. Could you elaborate a bit more if this is not expected? https://codereview.chromium.org/27498002/diff/4001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder_test.cc:90: media::VideoCodecProfile profile = media::H264PROFILE_HIGH; On 2013/11/21 06:31:41, posciak wrote: > On 2013/10/21 09:33:05, chihchung wrote: > > On 2013/10/20 23:53:11, posciak wrote: > > > Shouldn't profile be a parameter of the test, depending on the file? > > > > Yes, I just picked a value which looks like a larger subset of H.264 to avoid > > the trouble parsing the stream, and it seems to work fine for base/main > profile > > streams also. > > > > Right, but did you test the other way around? In Chrome we select the profile, > so for base we use base, for main we use main. If you use high for everything it > may work well, but you may not catch a problem with using base for base for > example. > > > I don't really understand this parameter in VA API. Maybe VA API can use this > > parameter to reserved less resources if given profile needs less, or fails > > immediately if some profile is not supported. > > Most probably. Digging in libva-intel-driver would probably give you an answer. Grepping VAProfileH264 in libva-intel-driver, there is no difference at all in handling Baseline/Main/High... https://codereview.chromium.org/27498002/diff/38001/content/common/gpu/media/... File content/common/gpu/media/vaapi_h264_decoder_test.cc (right): https://codereview.chromium.org/27498002/diff/38001/content/common/gpu/media/... content/common/gpu/media/vaapi_h264_decoder_test.cc:65: // These members (x_display_, output_file_, picture_count_, num_surfaces_) On 2013/11/21 06:31:41, posciak wrote: > picture_count_ and num_surfaces are not really freed. And you don't really zero > them out either, apart from on construction. Clarified a bit. https://codereview.chromium.org/27498002/diff/38001/content/common/gpu/media/... content/common/gpu/media/vaapi_h264_decoder_test.cc:69: int picture_count_; // number of pictures already outputted On 2013/11/21 06:31:41, posciak wrote: > s/picture_count_/num_outputted_pictures_ Done. https://codereview.chromium.org/27498002/diff/38001/content/common/gpu/media/... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/27498002/diff/38001/content/common/gpu/media/... content/common/gpu/media/vaapi_wrapper.cc:420: void* mem) { On 2013/11/21 06:31:41, posciak wrote: > What do you think of using VideoFrame::WrapExternalYuvData, saving to disk and > then releasing this, without copying? You don't return them immediately anyway. > > Not a big deal though. I don't understand this, but you said it's not a big deal :) https://codereview.chromium.org/27498002/diff/38001/content/common/gpu/media/... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/27498002/diff/38001/content/common/gpu/media/... content/common/gpu/media/vaapi_wrapper.h:85: scoped_refptr<media::VideoFrame> GetI420FromSurface( On 2013/11/21 06:31:41, posciak wrote: > Rename to VideoFrameFromVASurface or something like that. Done.
Change it to a unit test and add MD5 sum, so we don't need the full content for verification. Please take a look. Thanks!
Sorry it took me so long. https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... File content/common/gpu/media/vaapi_h264_decoder_unittest.cc (right): https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:19: // DISPLAY=:0 ./vaapi_h264_run_decoder --input_file input.264 s/vaapi_h264_run_decoder/vaapi_h264_decoder_unittest/ https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:255: input_file = base::FilePath("test-25fps.h264"); Please consider putting the default filenams as a const at the top of the file and overwrite if a param is passed like we do in vea/vda test. Easier to spot I feel. https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:256: md5sum = "3af866863225b956001252ebeccdb71d"; Ditto. https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:267: LOG(FATAL) << "initialize decoder loop failed"; This should fail the test instead (ASSERT_TRUE). In general, I recommend replace most if not all if (!foo.Bar()) failures in this test with either EXPECT_* or ASSERT_* so that we fail the test and have meaningful messages at failure site. Should be no need to use LOG(ERROR) then also. https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:287: // Needed to enable DVLOG through --vmodule. You don't have a single DVLOG in this file though ;) It's probably better to use DVLOG than VLOG. https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:117: typedef VAStatus (*VaapiMapBuffer)(VADisplay dpy, Please maintain lexicographical order. https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:448: static scoped_refptr<media::VideoFrame> CopyNV12ToI420(VAImage* image, To be honest I feel now that all the conversion code should go to the test file. We don't use it outside the test and it'd be better not to have it in the compiled Chrome. We should keep test-related changes in the wrapper to the minimum. Since the test has to understand vaapi anyway, just maybe have a VaapiWrapper::GetVaImage to derive image here returning VAImage and also a VW::ReturnVaImage to free it and do conversions and copies outside of wrapper. What do you think? https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:458: media::VideoFrame::CreateFrame(media::VideoFrame::I420, Normally I'd suggest adding a VideoFrame::WrapVAImage(), but it's probably a better idea to contain this here and not pollute VideoFrame with code used just for testing. https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:478: if ((i & 1) == 1) It's obvious for us, but maybe add a comment why this is. https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:481: dstU[k] = srcU[j]; Did a quick peek into vagetimage and it looks like it doesn't do image conversion based on given image.format. You'd have to use VPP infra in vaapi. Not worth it. https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:502: scoped_refptr<media::VideoFrame> result; s/result/frame or something like that :) https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:507: if (va_res == VA_STATUS_SUCCESS) { Early return from here instead maybe to save on indents? https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:519: LOG(ERROR) << "unexpected image format: " << image.format.fourcc; Please use DVLOG() instead so as to not pollute the logs and fail at call site. https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:87: // NV12 format. Perhaps it'd be a good idea to add a note that this function is and should only be used for testing in case someone decides to use it somewhere meaningful, because it makes a copy. https://codereview.chromium.org/27498002/diff/188001/content/content_tests.gypi File content/content_tests.gypi (right): https://codereview.chromium.org/27498002/diff/188001/content/content_tests.gy... content/content_tests.gypi:1262: ['(chromeos==1 or OS=="linux") and target_arch != "arm"', { I think we don't want to have linux here. Also we want x11: I would suggest: 'chromeos == 1 and use_x11 == 1 and 'target_arch != "arm"' https://codereview.chromium.org/27498002/diff/188001/content/content_tests.gy... content/content_tests.gypi:1277: '<(DEPTH)/third_party/libva', I think this should already be included and you don't use libva directly.
https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... File content/common/gpu/media/vaapi_h264_decoder_unittest.cc (right): https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:19: // DISPLAY=:0 ./vaapi_h264_run_decoder --input_file input.264 On 2013/11/29 04:25:13, posciak wrote: > s/vaapi_h264_run_decoder/vaapi_h264_decoder_unittest/ Done. https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:255: input_file = base::FilePath("test-25fps.h264"); On 2013/11/29 04:25:13, posciak wrote: > Please consider putting the default filenams as a const at the top of the file > and overwrite if a param is passed like we do in vea/vda test. Easier to spot I > feel. Done. https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:256: md5sum = "3af866863225b956001252ebeccdb71d"; On 2013/11/29 04:25:13, posciak wrote: > Ditto. Done. https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:267: LOG(FATAL) << "initialize decoder loop failed"; On 2013/11/29 04:25:13, posciak wrote: > This should fail the test instead (ASSERT_TRUE). > > In general, I recommend replace most if not all if (!foo.Bar()) failures in this > test with either EXPECT_* or ASSERT_* so that we fail the test and have > meaningful messages at failure site. > Should be no need to use LOG(ERROR) then also. Done. https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:287: // Needed to enable DVLOG through --vmodule. On 2013/11/29 04:25:13, posciak wrote: > You don't have a single DVLOG in this file though ;) > It's probably better to use DVLOG than VLOG. I don't really understand this magic :) I think I'll keep VLOG as this is not production code, so performance is not too important. https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:117: typedef VAStatus (*VaapiMapBuffer)(VADisplay dpy, On 2013/11/29 04:25:13, posciak wrote: > Please maintain lexicographical order. Done. https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:448: static scoped_refptr<media::VideoFrame> CopyNV12ToI420(VAImage* image, On 2013/11/29 04:25:13, posciak wrote: > To be honest I feel now that all the conversion code should go to the test file. > We don't use it outside the test and it'd be better not to have it in the > compiled Chrome. We should keep test-related changes in the wrapper to the > minimum. > > Since the test has to understand vaapi anyway, just maybe have a > VaapiWrapper::GetVaImage to derive image here returning VAImage and also a > VW::ReturnVaImage to free it and do conversions and copies outside of wrapper. > > What do you think? I prefer current code because (1) The linker should be able to remove unused functions for compiled chrome (2) The GetVaImage function will need to return two parameters, VAImage and void* mem, because the test code doesn't have access to VAAPI_MapBuffer. But I am OK to change it if you think it's better. https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:478: if ((i & 1) == 1) On 2013/11/29 04:25:13, posciak wrote: > It's obvious for us, but maybe add a comment why this is. Done. https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:502: scoped_refptr<media::VideoFrame> result; On 2013/11/29 04:25:13, posciak wrote: > s/result/frame or something like that :) Done. https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:507: if (va_res == VA_STATUS_SUCCESS) { On 2013/11/29 04:25:13, posciak wrote: > Early return from here instead maybe to save on indents? Done. https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:519: LOG(ERROR) << "unexpected image format: " << image.format.fourcc; On 2013/11/29 04:25:13, posciak wrote: > Please use DVLOG() instead so as to not pollute the logs and fail at call site. Done. https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:87: // NV12 format. On 2013/11/29 04:25:13, posciak wrote: > Perhaps it'd be a good idea to add a note that this function is and should only > be used for testing in case someone decides to use it somewhere meaningful, > because it makes a copy. Done. https://codereview.chromium.org/27498002/diff/188001/content/content_tests.gypi File content/content_tests.gypi (right): https://codereview.chromium.org/27498002/diff/188001/content/content_tests.gy... content/content_tests.gypi:1262: ['(chromeos==1 or OS=="linux") and target_arch != "arm"', { On 2013/11/29 04:25:13, posciak wrote: > I think we don't want to have linux here. Also we want x11: > I would suggest: > > 'chromeos == 1 and use_x11 == 1 and 'target_arch != "arm"' Done. https://codereview.chromium.org/27498002/diff/188001/content/content_tests.gy... content/content_tests.gypi:1277: '<(DEPTH)/third_party/libva', On 2013/11/29 04:25:13, posciak wrote: > I think this should already be included and you don't use libva directly. Done.
https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... File content/common/gpu/media/vaapi_h264_decoder_unittest.cc (right): https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:267: LOG(FATAL) << "initialize decoder loop failed"; On 2013/11/29 14:06:30, chihchung wrote: > On 2013/11/29 04:25:13, posciak wrote: > > This should fail the test instead (ASSERT_TRUE). > > > > In general, I recommend replace most if not all if (!foo.Bar()) failures in > this > > test with either EXPECT_* or ASSERT_* so that we fail the test and have > > meaningful messages at failure site. > > Should be no need to use LOG(ERROR) then also. > > Done. I said ASSERT/REQUIRE, not CHECK :) CHECK is a debug macro which is used like assert() from C. It will simply crash us. ASSERT_*() are gtest macros, which are used to fail the test properly. https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:287: // Needed to enable DVLOG through --vmodule. On 2013/11/29 14:06:30, chihchung wrote: > On 2013/11/29 04:25:13, posciak wrote: > > You don't have a single DVLOG in this file though ;) > > It's probably better to use DVLOG than VLOG. > > I don't really understand this magic :) > I think I'll keep VLOG as this is not production code, so performance is not too > important. It's more about reducing the amount of output from the test for successful runs. If this test is to be run on bots (and I hope it will be), it'd produce lots output even on successful runs and logs will be unnecessarily big. Although we still need to --vmodule/--v to enable vlogs, so I guess it's ok. https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:448: static scoped_refptr<media::VideoFrame> CopyNV12ToI420(VAImage* image, On 2013/11/29 14:06:30, chihchung wrote: > On 2013/11/29 04:25:13, posciak wrote: > > To be honest I feel now that all the conversion code should go to the test > file. > > We don't use it outside the test and it'd be better not to have it in the > > compiled Chrome. We should keep test-related changes in the wrapper to the > > minimum. > > > > Since the test has to understand vaapi anyway, just maybe have a > > VaapiWrapper::GetVaImage to derive image here returning VAImage and also a > > VW::ReturnVaImage to free it and do conversions and copies outside of wrapper. > > > > What do you think? > > I prefer current code because (1) The linker should be able to remove unused > functions for compiled chrome (2) The GetVaImage function will need to return > two parameters, VAImage and void* mem, because the test code doesn't have access > to VAAPI_MapBuffer. But I am OK to change it if you think it's better. 1) We just did an interesting experiment where we found out that those methods are indeed optimized out, but their string literals are kept (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=192). I would still insist on removing CopyNV12ToI420 from this class, even if for code clarity. Pretty please? :) 2) True, but unfortunately necessary I'd say. (Technically you could instead wrap NV12 into VideoFrame and convert later, but VF doesn't have support for NV12 and it's probably not a good idea to add it just for this test). https://codereview.chromium.org/27498002/diff/228001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/27498002/diff/228001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:523: DVLOG(ERROR) << "unexpected image format: " << image.format.fourcc; The convention is to use numeric constants for *VLOG()s.
Hi Ami, Please take a look at this test. Thanks! https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... File content/common/gpu/media/vaapi_h264_decoder_unittest.cc (right): https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:267: LOG(FATAL) << "initialize decoder loop failed"; On 2013/12/03 08:52:06, posciak wrote: > On 2013/11/29 14:06:30, chihchung wrote: > > On 2013/11/29 04:25:13, posciak wrote: > > > This should fail the test instead (ASSERT_TRUE). > > > > > > In general, I recommend replace most if not all if (!foo.Bar()) failures in > > this > > > test with either EXPECT_* or ASSERT_* so that we fail the test and have > > > meaningful messages at failure site. > > > Should be no need to use LOG(ERROR) then also. > > > > Done. > > I said ASSERT/REQUIRE, not CHECK :) > CHECK is a debug macro which is used like assert() from C. It will simply crash > us. > ASSERT_*() are gtest macros, which are used to fail the test properly. Done. https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/27498002/diff/188001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:448: static scoped_refptr<media::VideoFrame> CopyNV12ToI420(VAImage* image, On 2013/12/03 08:52:06, posciak wrote: > On 2013/11/29 14:06:30, chihchung wrote: > > On 2013/11/29 04:25:13, posciak wrote: > > > To be honest I feel now that all the conversion code should go to the test > > file. > > > We don't use it outside the test and it'd be better not to have it in the > > > compiled Chrome. We should keep test-related changes in the wrapper to the > > > minimum. > > > > > > Since the test has to understand vaapi anyway, just maybe have a > > > VaapiWrapper::GetVaImage to derive image here returning VAImage and also a > > > VW::ReturnVaImage to free it and do conversions and copies outside of > wrapper. > > > > > > What do you think? > > > > I prefer current code because (1) The linker should be able to remove unused > > functions for compiled chrome (2) The GetVaImage function will need to return > > two parameters, VAImage and void* mem, because the test code doesn't have > access > > to VAAPI_MapBuffer. But I am OK to change it if you think it's better. > > 1) We just did an interesting experiment where we found out that those methods > are indeed optimized out, but their string literals are kept > (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=192). > I would still insist on removing CopyNV12ToI420 from this class, even if for > code clarity. Pretty please? :) > > 2) True, but unfortunately necessary I'd say. (Technically you could instead > wrap NV12 into VideoFrame and convert later, but VF doesn't have support for > NV12 and it's probably not a good idea to add it just for this test). Done. https://codereview.chromium.org/27498002/diff/228001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/27498002/diff/228001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:523: DVLOG(ERROR) << "unexpected image format: " << image.format.fourcc; On 2013/12/03 08:52:06, posciak wrote: > The convention is to use numeric constants for *VLOG()s. Moved to test and changed to LOG.
I'm probably missing some important context :( https://codereview.chromium.org/27498002/diff/248001/content/common/gpu/media... File content/common/gpu/media/vaapi_h264_decoder_unittest.cc (right): https://codereview.chromium.org/27498002/diff/248001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:24: // expected MD5 sum is given. Is it easy to see why this should be a separate binary and not part of vdatest, which seems like almost completely a superset, functionally? What's the use-case for this binary? (does the same use-case not apply to vp8/evda/dxvavda/etc?) https://codereview.chromium.org/27498002/diff/248001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/27498002/diff/248001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:85: // Get a VAImage from a VASurface and map it into memory. The VAImage should If these APIs are only used for testing they should probably have a ForTesting suffix.
https://codereview.chromium.org/27498002/diff/248001/content/common/gpu/media... File content/common/gpu/media/vaapi_h264_decoder_unittest.cc (right): https://codereview.chromium.org/27498002/diff/248001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:24: // expected MD5 sum is given. On 2013/12/03 21:17:16, Ami Fischman wrote: > Is it easy to see why this should be a separate binary and not part of vdatest, > which seems like almost completely a superset, functionally? > > What's the use-case for this binary? (does the same use-case not apply to > vp8/evda/dxvavda/etc?) Hi, Sorry I should have made the context more clear: (1) It produces and compares the YUV data instead of RGB data. This avoids colorspace conversion / scaling differences across platforms like we have for vda test. (2) It tests decoder api only and not graphics api, so it may help isolate issues. (3) This is intended for regression testing and decoder improvement using the H.264.1 test suite. The test suite contains a couple hundred H.264 streams and its corresponding decoded YUV data. The test suite is publicly available but not re-distributable, so I will write a private ChromeOS test and verify there is no regression for these streams. https://codereview.chromium.org/27498002/diff/248001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/27498002/diff/248001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:85: // Get a VAImage from a VASurface and map it into memory. The VAImage should On 2013/12/03 21:17:16, Ami Fischman wrote: > If these APIs are only used for testing they should probably have a ForTesting > suffix. Done.
> > (1) It produces and compares the YUV data instead of RGB data. This > avoids colorspace conversion / scaling differences across platforms like > we have for vda test. > The key bit I missed is that VAVDA emits RGB unconditionally so this binary operates against VaapiH264Decoder instead of VaapiVDA to get the pre-CSC YUV. The alternative would be to make VAVDA aware of YUV textures (not RGB pixmaps, which is what it traffics in today), but that's a much bigger fish to fry. Taking a closer look at the code now. > https://codereview.chromium.org/27498002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/27498002/diff/268001/content/common/gpu/media... File content/common/gpu/media/vaapi_h264_decoder_unittest.cc (right): https://codereview.chromium.org/27498002/diff/268001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:19: // DISPLAY=:0 ./vaapi_h264_decoder_unittest --input_file input.264 s/264/h264/ to line up with kDefaultInputFile https://codereview.chromium.org/27498002/diff/268001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:20: // [--output_file output.yuv] [--md5sum expected_md5_hex] Can you s/yuv/i420/g throughout this file, for precision? https://codereview.chromium.org/27498002/diff/268001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:41: // frames (in I420 format) to another file. If you emitted http://wiki.multimedia.cx/index.php?title=YUV4MPEG2 then various tools could play back the results more easily (and the container should be trivial to hand-craft). Up to you. https://codereview.chromium.org/27498002/diff/268001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:44: // input and output file path, then call Run() to decode the whole stream and s/path/paths/ https://codereview.chromium.org/27498002/diff/268001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:65: void ReportToUMA(VaapiH264Decoder::VAVDAH264DecoderFailure error) {} s/ReportToUMA/BlackholeErrorCodes/ s/error// although TBH I'd prefer void CrashOnError(VaapiH264Decoder::VAVDAH264DecoderFailure error) { LOG(FATAL) << "Oh noes! Decoder failed: " << error; } There's also no need for this to be a member function (could just as well be a free file-static function), which would allow you to drop a base::Unretained() below. Unretained is generally a code-smell unless you have rationale about why it is safe. https://codereview.chromium.org/27498002/diff/268001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:104: // We need to destruct decoder and wrapper first because: Then wrapper_ should be declared before decoder_ in the class layout (C++ destruction works from the bottom up, so manual tear-down should work similarly for minimum confusion). https://codereview.chromium.org/27498002/diff/268001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:120: return false; In general for programs like these I prefer LOG(FATAL) to LOG(ERROR)+return false because it simplifies code everywhere (both caller & impl). E.g. l.117-121 can just be: CHECK(x_display_ = XOpenDisplay(NULL)); If you take this suggestions then the contents of Initialize() can migrate into the ctor. https://codereview.chromium.org/27498002/diff/268001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:123: media::VideoCodecProfile profile = media::H264PROFILE_HIGH; Doesn't this need to be a cmd-line param in case different inputs use different profiles? https://codereview.chromium.org/27498002/diff/268001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:185: std::string VaapiH264DecoderLoop::GetMD5Sum() { static https://codereview.chromium.org/27498002/diff/268001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:191: scoped_refptr<media::VideoFrame> CopyNV12ToI420(VAImage* image, void* mem) { libyuv::NV12ToI420 ? https://codereview.chromium.org/27498002/diff/268001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:244: base::MD5Update(&md5_context_, base::StringPiece(buf, bytes)); The MD5 part of this function can be satisfied instead by VideoFrame::HashFrameForTesting(). The rest of this function appears to be written to deal with non-zero padding / alignment / stride concerns, but l.195-197 say that's not something you're worried about, so I think you can collapse this function to simply: frame->HashFrameForTesting(&md5_context_); int to_write = frame->AllocationSize(frame->format(), frame->coded_size(); int written = file_util::AppendToFile(output_file_, frame->data(kYPlane), to_write); CHECK_EQ(written, to_write); ?
Thanks a lot for the review and suggestions! Please take another look. Thanks! https://codereview.chromium.org/27498002/diff/268001/content/common/gpu/media... File content/common/gpu/media/vaapi_h264_decoder_unittest.cc (right): https://codereview.chromium.org/27498002/diff/268001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:19: // DISPLAY=:0 ./vaapi_h264_decoder_unittest --input_file input.264 On 2013/12/05 02:11:43, Ami Fischman wrote: > s/264/h264/ to line up with kDefaultInputFile Done. https://codereview.chromium.org/27498002/diff/268001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:20: // [--output_file output.yuv] [--md5sum expected_md5_hex] On 2013/12/05 02:11:43, Ami Fischman wrote: > Can you s/yuv/i420/g throughout this file, for precision? Done. https://codereview.chromium.org/27498002/diff/268001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:41: // frames (in I420 format) to another file. On 2013/12/05 02:11:43, Ami Fischman wrote: > If you emitted http://wiki.multimedia.cx/index.php?title=YUV4MPEG2 then various > tools could play back the results more easily (and the container should be > trivial to hand-craft). Up to you. I use this command line to play it: mplayer test_dec.yuv -demuxer rawvideo -rawvideo w=1920:h=1080 The raw i420 format is to match the golden result in the conformance test. https://codereview.chromium.org/27498002/diff/268001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:44: // input and output file path, then call Run() to decode the whole stream and On 2013/12/05 02:11:43, Ami Fischman wrote: > s/path/paths/ Done. https://codereview.chromium.org/27498002/diff/268001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:65: void ReportToUMA(VaapiH264Decoder::VAVDAH264DecoderFailure error) {} On 2013/12/05 02:11:43, Ami Fischman wrote: > s/ReportToUMA/BlackholeErrorCodes/ > s/error// > > although TBH I'd prefer > void CrashOnError(VaapiH264Decoder::VAVDAH264DecoderFailure error) { > LOG(FATAL) << "Oh noes! Decoder failed: " << error; > } > > There's also no need for this to be a member function (could just as well be a > free file-static function), which would allow you to drop a base::Unretained() > below. Unretained is generally a code-smell unless you have rationale about > why it is safe. Done. https://codereview.chromium.org/27498002/diff/268001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:104: // We need to destruct decoder and wrapper first because: On 2013/12/05 02:11:43, Ami Fischman wrote: > Then wrapper_ should be declared before decoder_ in the class layout (C++ > destruction works from the bottom up, so manual tear-down should work similarly > for minimum confusion). Done. https://codereview.chromium.org/27498002/diff/268001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:120: return false; On 2013/12/05 02:11:43, Ami Fischman wrote: > In general for programs like these I prefer LOG(FATAL) to LOG(ERROR)+return > false because it simplifies code everywhere (both caller & impl). E.g. > l.117-121 can just be: > CHECK(x_display_ = XOpenDisplay(NULL)); > > If you take this suggestions then the contents of Initialize() can migrate into > the ctor. I think there might be a chance for this class to be used in a larger program, so I think we can leave it as is now. https://codereview.chromium.org/27498002/diff/268001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:123: media::VideoCodecProfile profile = media::H264PROFILE_HIGH; On 2013/12/05 02:11:43, Ami Fischman wrote: > Doesn't this need to be a cmd-line param in case different inputs use different > profiles? I checked the libva-intel-driver code, and it doesn't distinguish profiles in H.264 at all (it only distinguish things like H.264 vs MPEG2). https://codereview.chromium.org/27498002/diff/268001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:185: std::string VaapiH264DecoderLoop::GetMD5Sum() { On 2013/12/05 02:11:43, Ami Fischman wrote: > static It uses a md5_context_ member variable. https://codereview.chromium.org/27498002/diff/268001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:244: base::MD5Update(&md5_context_, base::StringPiece(buf, bytes)); On 2013/12/05 02:11:43, Ami Fischman wrote: > The MD5 part of this function can be satisfied instead by > VideoFrame::HashFrameForTesting(). > > The rest of this function appears to be written to deal with non-zero padding / > alignment / stride concerns, but l.195-197 say that's not something you're > worried about, so I think you can collapse this function to simply: > frame->HashFrameForTesting(&md5_context_); > int to_write = frame->AllocationSize(frame->format(), frame->coded_size(); > int written = file_util::AppendToFile(output_file_, frame->data(kYPlane), > to_write); > CHECK_EQ(written, to_write); > ? Done. Thanks a lot for the suggestion. I use one write call for each plane, because I am not sure there is no paddings between the three planes.
LGTM % nits https://codereview.chromium.org/27498002/diff/268001/content/common/gpu/media... File content/common/gpu/media/vaapi_h264_decoder_unittest.cc (right): https://codereview.chromium.org/27498002/diff/268001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:120: return false; On 2013/12/09 05:10:08, chihchung wrote: > On 2013/12/05 02:11:43, Ami Fischman wrote: > > In general for programs like these I prefer LOG(FATAL) to LOG(ERROR)+return > > false because it simplifies code everywhere (both caller & impl). E.g. > > l.117-121 can just be: > > CHECK(x_display_ = XOpenDisplay(NULL)); > > > > If you take this suggestions then the contents of Initialize() can migrate > into > > the ctor. > > I think there might be a chance for this class to be used in a larger program, I hope not! :) (libva as an API is pretty terrible; I hope we don't end up with more dependencies on it at this level, rather than the VDA level) https://codereview.chromium.org/27498002/diff/268001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:123: media::VideoCodecProfile profile = media::H264PROFILE_HIGH; On 2013/12/09 05:10:08, chihchung wrote: > On 2013/12/05 02:11:43, Ami Fischman wrote: > > Doesn't this need to be a cmd-line param in case different inputs use > different > > profiles? > > I checked the libva-intel-driver code, and it doesn't distinguish profiles in > H.264 at all (it only distinguish things like H.264 vs MPEG2). I think you mean you checked the implementation, but this file doesn't use the libva implementation; it uses its interface. It would be nice to avoid hard-coding today's libva's impl details into this test for the future. E.g. of where this could bite you: libva impl is changed to allocate only resources necessary for the specified profile. A baseline profile video incorrectly requires too many resources. This test wouldn't catch that b/c it was still asking for HIGH when it should be asking for BASELINE. https://codereview.chromium.org/27498002/diff/288001/content/common/gpu/media... File content/common/gpu/media/vaapi_h264_decoder_unittest.cc (right): https://codereview.chromium.org/27498002/diff/288001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:42: // frames (in I420 format) to another file. Will probably be useful to drop a hint here about how to play the resulting file (e.g. your mplayer test_dec.yuv -demuxer rawvideo -rawvideo w=1920:h=1080 ) https://codereview.chromium.org/27498002/diff/288001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:115: LOG(ERROR) << "Oh noes! Decoder failed: " << error; The choice of ERROR here is effectively equivalent to silence b/c the error is not acted upon. Given this is going to be running as an automated test suite (not a manual test thing whose logs are inspected by humans) I think LOG(FATAL) is the only reasonable way to go here.
+jochen for content/content_tests.gypi owner approval. https://codereview.chromium.org/27498002/diff/268001/content/common/gpu/media... File content/common/gpu/media/vaapi_h264_decoder_unittest.cc (right): https://codereview.chromium.org/27498002/diff/268001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:123: media::VideoCodecProfile profile = media::H264PROFILE_HIGH; I changed it to baseline, so if the implementation changes, we can easily catch that. On 2013/12/09 18:55:56, Ami Fischman (OOO Dec23-Jan5) wrote: > On 2013/12/09 05:10:08, chihchung wrote: > > On 2013/12/05 02:11:43, Ami Fischman wrote: > > > Doesn't this need to be a cmd-line param in case different inputs use > > different > > > profiles? > > > > I checked the libva-intel-driver code, and it doesn't distinguish profiles in > > H.264 at all (it only distinguish things like H.264 vs MPEG2). > > I think you mean you checked the implementation, but this file doesn't use the > libva implementation; it uses its interface. It would be nice to avoid > hard-coding today's libva's impl details into this test for the future. > > E.g. of where this could bite you: libva impl is changed to allocate only > resources necessary for the specified profile. A baseline profile video > incorrectly requires too many resources. This test wouldn't catch that b/c it > was still asking for HIGH when it should be asking for BASELINE. https://codereview.chromium.org/27498002/diff/288001/content/common/gpu/media... File content/common/gpu/media/vaapi_h264_decoder_unittest.cc (right): https://codereview.chromium.org/27498002/diff/288001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:42: // frames (in I420 format) to another file. On 2013/12/09 18:55:56, Ami Fischman (OOO Dec23-Jan5) wrote: > Will probably be useful to drop a hint here about how to play the resulting file > (e.g. your > mplayer test_dec.yuv -demuxer rawvideo -rawvideo w=1920:h=1080 > ) Done. https://codereview.chromium.org/27498002/diff/288001/content/common/gpu/media... content/common/gpu/media/vaapi_h264_decoder_unittest.cc:115: LOG(ERROR) << "Oh noes! Decoder failed: " << error; On 2013/12/09 18:55:56, Ami Fischman (OOO Dec23-Jan5) wrote: > The choice of ERROR here is effectively equivalent to silence b/c the error is > not acted upon. Given this is going to be running as an automated test suite > (not a manual test thing whose logs are inspected by humans) I think LOG(FATAL) > is the only reasonable way to go here. Done.
lgtm
The CQ bit was checked by chihchung@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chihchung@chromium.org/27498002/438001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_chromeos_clang for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
The CQ bit was checked by chihchung@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chihchung@chromium.org/27498002/718001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_chromeos_clang for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
The CQ bit was checked by chihchung@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chihchung@chromium.org/27498002/1018001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_chromeos_clang for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
The CQ bit was checked by chihchung@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chihchung@chromium.org/27498002/1298001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_chromeos_clang for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
The CQ bit was checked by chihchung@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chihchung@chromium.org/27498002/1588001
Message was sent while issue was closed.
Change committed as 250987 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
