Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6961018:
Pepper Video Decoder API tester plugin. (Closed)
|
Created:
9 years, 7 months ago by Ville-Mikko Rautio Modified:
9 years, 6 months ago Reviewers:
vrk (LEFT CHROMIUM), Ami GONE FROM CHROMIUM, vjain, vhiremath, mheikkinen1, piman, ashok, cfreeman, vmr, shikhas, Ami GONE FROM WEBRTC_CHROMIUM Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPepper Video Decoder API tester plugin.
BUG=
TEST=make ppapi_tests
Patch Set 1 #Patch Set 2 : Refactoring #Patch Set 3 : Commenting and clean-up #Patch Set 4 : Simple lint fixes. #
Total comments: 7
Patch Set 5 : Separated parts of tester into examples and use those files. #Patch Set 6 : More implementation meat and clearing things all around. #
Total comments: 84
Patch Set 7 : Codereview comments taken into account. #Patch Set 8 : Rebase & compilation fixes related to it. #
Messages
Total messages: 20 (0 generated)
Hello everyone, I uploaded initial version of the Pepper Video Decoder API tester plugin for review. I would like to have especially feedback on general design and VideoBitstreamSource and DisplayInterface interface's applicability to Flash and other applications that could use it. Next I will be moving on to finishing the implementation of this and testing on Kaen hardware. Ville-Mikko
My main feedback is that it's unclear to me that the abstract interfaces introduced in this CL actually make it easier to write a plugin that uses the video decode API. ISTM this actually makes the amount of code a plugin writer needs to look at, and the number of abstractions involved, significantly larger. I had some draft comments while looking through the CL so I'm including them below but I stopped reviewing for code once I realized what you were doing (and the above concern came up), around the declaration of DisplayInterface. http://codereview.chromium.org/6961018/diff/11001/ppapi/cpp/dev/video_decoder... File ppapi/cpp/dev/video_decoder_dev.cc (right): http://codereview.chromium.org/6961018/diff/11001/ppapi/cpp/dev/video_decoder... ppapi/cpp/dev/video_decoder_dev.cc:49: PP_VideoConfigElement* proto_config_array = scoped_array? http://codereview.chromium.org/6961018/diff/11001/ppapi/cpp/dev/video_decoder... ppapi/cpp/dev/video_decoder_dev.cc:61: // Then get the actual configs and restore it in a vector. s/restore it/store them/ http://codereview.chromium.org/6961018/diff/11001/ppapi/cpp/dev/video_decoder... ppapi/cpp/dev/video_decoder_dev.cc:63: = new PP_VideoConfigElement[num_of_matching_configs]; indent http://codereview.chromium.org/6961018/diff/11001/ppapi/cpp/dev/video_decoder... File ppapi/cpp/dev/video_decoder_dev.h (right): http://codereview.chromium.org/6961018/diff/11001/ppapi/cpp/dev/video_decoder... ppapi/cpp/dev/video_decoder_dev.h:100: PPB_VideoDecoder_Dev* ppb_videodecoder_dev_; Unused? http://codereview.chromium.org/6961018/diff/11001/ppapi/ppapi_tests.gypi File ppapi/ppapi_tests.gypi (right): http://codereview.chromium.org/6961018/diff/11001/ppapi/ppapi_tests.gypi#newc... ppapi/ppapi_tests.gypi:247: 'include_dirs': [ What's with these changes? http://codereview.chromium.org/6961018/diff/11001/ppapi/tests/test_video_deco... File ppapi/tests/test_video_decoder.h (right): http://codereview.chromium.org/6961018/diff/11001/ppapi/tests/test_video_deco... ppapi/tests/test_video_decoder.h:90: // Writes decodable bitstream unit to |target_mem| which has size of What's a "unit"? http://codereview.chromium.org/6961018/diff/11001/ppapi/tests/test_video_deco... ppapi/tests/test_video_decoder.h:95: int32_t* unit_size_in_bytes) = 0; Why signed?
Hi Ami and thank you for the comments. I gave some thought on the usability of the VideoDecoderClient from the plugin's perspective. With this new abstraction class plugin developer must worry about using the following 5 functions with no parameters from the VideoDecoderClient: // Functions to control the execution. bool Initialize(); bool Run(); bool Stop(); bool Flush(); bool Teardown(); In addition plugin developer needs to implement VideoBitstreamInterface (with 1 simple "stateless" functions) and DisplayInterface (with 4 functions of which 1 generates a callback). If using "raw" C/CPP API there's 7 functions (with many parameters) to use (of which and 3 of them have one time callback) and 5 callbacks methods to implement. Also the raw API requires a lot more state information to be contained within the developer's plugin about the progress with the API. So in summary, I believe this approach adds value by reducing complexity that plugin developer has to handle quite a bit for normal decoding operation. However, I was thinking that I might refactor the VideoDecoderClient under ppapi/examples folder as a example and either use that example in the tester or just implement raw tests that sit directly on top of the API. However, if the latter is the case, I would have to implement much of the same state information in the tester anyway to control the decoding flow correctly. I will make the fixes to your draft comments once we clear out on direction of this code. Let me know how you feel about it now after sleeping over night. Thanks, Ville-Mikko
Added Christopher and Vikas as reviewers of this issue, as you might want to reuse some of the code. Also, separated parts of tester under examples and now tester would be using parts of the examples.
On 2011/05/27 13:37:48, vmr1 wrote: > Added Christopher and Vikas as reviewers of this issue, as you might want to > reuse some of the code. > > Also, separated parts of tester under examples and now tester would be using > parts of the examples. Hi Ville-Mikko, While scanning through the code, I could not find any code to handle video timestamps (TS) in the video decoder interface. Can you please point me to the piece of code that handles it? If the TS handling is missing, how are we planning to implement this to achieve AVsync?
Hi Vikas, Functionality is there but it needs some explanation. When supplying the bitstream data plugin application knows the metadata (timestamp among other things) related to that bitstream buffer. It needs to store the metadata (and the timestamp) in the application logic and associate the bitstream buffer id to be decoded. When the picture is ready the delivered picture structure has bitstream_buffer_id field, which identifies the buffer that the picture was decoded from. Application should use that information to retrieve the metadata (including the timestamp) and act accordingly with the output picture. So in summary, API itself does not care about any timestamps, it just decodes as fast as possible and it is up to the application to decide when it displays the picture. I hope this clarifies the situation. Ville-Mikko
Current state of the PPAPI tester has also been updated. Goal is to get the thing decoding H.264 bitstream at the end of the week. Output is conditional to actual decoder working and getting the contexts+textures sorted out correctly all the way to the gpu process.
On 2011/05/30 10:13:29, vmr1 wrote: > Hi Vikas, > > Functionality is there but it needs some explanation. > > When supplying the bitstream data plugin application knows the metadata > (timestamp among other things) related to that bitstream buffer. It needs to > store the metadata (and the timestamp) in the application logic and associate > the bitstream buffer id to be decoded. > > When the picture is ready the delivered picture structure has > bitstream_buffer_id field, which identifies the buffer that the picture was > decoded from. Application should use that information to retrieve the metadata > (including the timestamp) and act accordingly with the output picture. > > So in summary, API itself does not care about any timestamps, it just decodes as > fast as possible and it is up to the application to decide when it displays the > picture. > > I hope this clarifies the situation. > > Ville-Mikko Hi Ville-Mikko, In this case, each application/plugin will have to keep a map store of the timestamps ID assocition. IMO, it would be great if the video decoder handles this and app/plugin simply pass the input with timestamp and receive the output with timestamp. This will also allow decoder to decode as fast as possible. It can help by, 1) The timestamp ID association will be handled in the common code, in our case video decoder. 2) It will help the decoder to drop decoding of B frames in case on pre-rolling condition. Flash websites supports pre-rolling, after seeking. Apart from current discussion, are we planning to support a feature so that video decoder can support dropping of non-reference input frames, when the decoder is not able to maintain AV sync.
Hi Ville-Mikko, In file ppapi/examples/video_decoder/video_decoder_session.cc, functions VideoDecoderSession::Flush() and VideoDecoderSession::NotifyEndOfStream() uses 2 different prototype for video_decode_->Flush() callback. 1)How will gpu video decoder distinguish between 2 callback implementation? 2) Why we have 2 different callback prototype for same function? Vikas
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... File ppapi/examples/video_decoder/video_decoder_session.cc (right): http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:196: const std::vector<uint32_t>& buffer_properties) { What is the purpose of buffer_properties? Will GPU video decoder report the dimensions of the video or should the application/plug-in rely on the parameters it obtained from locally decoding SPS and PPS?
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... File ppapi/examples/video_decoder/video_decoder_session.cc (right): http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:196: const std::vector<uint32_t>& buffer_properties) { On 2011/05/31 22:29:07, vjain wrote: > What is the purpose of buffer_properties? > Will GPU video decoder report the dimensions of the video or should the > application/plug-in rely on the parameters it obtained from locally decoding SPS > and PPS? Buffer properties will contain width & height and the color format of the buffer. They will be determined as key-value pairs as defined in pp_video_dev.h.
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... File ppapi/examples/video_decoder/video_decoder_session.cc (right): http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:196: const std::vector<uint32_t>& buffer_properties) { On 2011/06/01 11:50:08, vmr1 wrote: > On 2011/05/31 22:29:07, vjain wrote: > > What is the purpose of buffer_properties? > > Will GPU video decoder report the dimensions of the video or should the > > application/plug-in rely on the parameters it obtained from locally decoding > SPS > > and PPS? > > Buffer properties will contain width & height and the color format of the > buffer. They will be determined as key-value pairs as defined in pp_video_dev.h. Actually, this C++ API is not up to date with the C API. Ville-Mikko's comment holds for the original design of the API. However, our approach now is to establish properties such as the output color format in the Create/Init() phase. Instead of this buffer_properties vector, we request dimensions and other properties as explicit parameters in ProvidePictureBuffers -- see ppp_video_decoder_dev.h.
Hi Victoria, Thanks for the update. In that case 1) Sample app must be updated so that it list correct prototype. 2) Also has NotifyEndOfStream() is renamed to EndOfStream()?
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... File ppapi/examples/video_decoder/video_decoder_session.cc (right): http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:252: void VideoDecoderSession::OnBitstreamBufferProcessed( Hi Ville-Mikko, When the GPU decoder returns the buffer id (of bitstream buffer), would it set the "size" data field to '0'? If not, do we plan to add this in the code?
My overall comment is you seem to be writing a hybrid between a plugin and a
test. For example:
- You have not written a plugin: nothing in video_decoder_session.{cc,h}
implements pp::Instance or pp::Module. (Please see
http://code.google.com/p/ppapi/wiki/GettingStarted for the requirements of a
plugin.)
- You have not written a test: a ppapi VideoDecoder unit test would test the
behavior of VideoDecoder_Dev to make sure the video decoder interface's
individual function calls are working properly. (Please see tests under
ppapi/tests/ for examples.)
It seems to me you are using the ppapi TestCase framework to glue all the pieces
of your VideoDecoderSession API together, which is a hack to get around
registering modules/instances as a plugin with the browser.
I think the best way to fix this is to either delete test_video_decoder.{cc,h}
and write an example plugin, or delete video_decoder_session.{cc,h} and write a
unit test.
From your comments, it seems like you want people to use the VideoDecoderSession
API for their video decoding purposes instead of directly using
pp::VideoDecoder. Thus I would suggest instead eliminating
test_video_decoder.{cc,h} and writing an example plugin that shows how to use
VideoDecoderSession.
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
File ppapi/examples/video_decoder/video_decoder_session.cc (right):
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:31: std::ifstream* file =
scoped_ptr, then delete the deletes below :)
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:53: int32_t Read(uint8_t*
target_mem, uint32_t size) {
It looks like you are doing some decoding to split up bitstream buffers based on
a start code of 0001. I don't understand why this needs to be done here; it
seems to me you should simply be passing raw buffers to the decoder, which will
do the decoding.
At the very least you need write some comments explaining what format you're
expecting the raw data to be in, but again it doesn't make sense to me why the
client needs to look at the bitstream like this.
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:61: // start code found
Complete sentence + explanation of start code.
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:65: // back-up 1 byte.
Complete sentence.
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:74: // Last unit. Set the
unit_end to point to the last byte.
The last buffer begins _and_ ends with 0001?
In other words the buffers look like:
buffer[0] = 0001<data>
buffer[1] = 0001<data>
...
buffer[n] = 0001<data>0001
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:94: uint8_t* mem_;
scoped_array? At the very least you need to delete mem_ in the destructor; right
now you're leaking.
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:104: delete video_source_;
No longer needed w/ scoped_ptr
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:119: if (read_bytes <= 0)
{
Nit: no {} for one-line condition
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:138: assert(video_source_
&& display_);
Do this check in the constructor.
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:140: if
(!AllocateInputBuffers()) {
Nit: No {}
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:146: if (!video_decoder_)
{
Nit: No {}
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:159: if
(!ReadAndDispatchBitstreamUnit((*it).first)) {
Nit: {}
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:289: buffer_if_ =
static_cast<const struct PPB_Buffer_Dev*>(
?? Why is this field being set here? Shouldn't this be set in the constructor?
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:291: if (!buffer_if_) {
Nit: no {}
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:300: if
(bitstream_buffer.data == 0) {
Nit: no {}
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:303: bitstream_buffer.size
= 0;
This should be kBitstreamBufferSize with change mentioned below (line 337).
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:330: if (target_mem ==
NULL) {
Nit: no {}
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:334: if
(!buffer_if_->Describe(bitstream_buffer.data, &size_in_bytes)) {
Nit: no {}
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:337: bool success =
video_source_->GetBitstreamUnit(target_mem, size_in_bytes,
It's wasteful to allocate a buffer of kBitstreamBufferSize, then copy data that
may be less than kBitstreamBufferSize into it.
But it seems like GetBitstreamUnit should always copy bitstream_buffer.size
bytes regardless, so if you fix the GetBitstreamUnit logic above then you no
longer have this problem.
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:339: if (!success) {
Nit: no {}
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:344:
bitstream_buffers_[bitstream_buffer_id],
bitstream_buffer?
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:349:
buffer_if_->Unmap(bitstream_buffer.data);
Unmap deletes the bitstream data. Shouldn't we only do this if
OnBitstreamBufferProcessed?
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:361: "precision highp
float; precision highp int;\n"
I don't believe these precision specifiers are necessary because you are not
using float or int in your shader.
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:374: "precision mediump
float;\n"
Ditto above.
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:392: if
(!InitGL(surface_size_.width, surface_size_.height)) {
nit: no {}
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:401: PP_GLESBuffer_Dev*
picture_buffer) {
Would make sense to also do the initial call to TexImage2D in this function to
actually allocate space on the texture.
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:425:
pp::CompletionCallback completion_callback) {
nit: add space before pp
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:435: // Draw the texture.
You never copy data into the texture (TexImage2D/MapTexSubImage2DCHROMIUM), so
you are not drawing anything.
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:444: if (error != PP_OK) {
nit: no {}
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:512: assert(result);
You never use log... change to fail with "log" as error message?
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:568: 8 *
sizeof(kVertices[0]), kVertices, GL_STREAM_DRAW);
Should this be GL_STATIC_DRAW?
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:583: kTextureCoordsEgl,
GL_STREAM_DRAW);
Ditto above.
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.cc:592:
gles2_if_->Enable(context_->pp_resource(), GL_DEPTH_TEST);
What is GL_DEPTH_TEST?
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
File ppapi/examples/video_decoder/video_decoder_session.h (right):
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.h:80: PP_GLESBuffer_Dev*
picture_buffer) = 0;
Why does this only provide one picture buffer at a time? Seems like
VideoDecoderSession should just forward its ProvidePictureBuffers command to
this function.
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.h:105: TestVideoSource*
video_source_;
scoped_ptr?
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.h:172: state_(kCreated) {}
Put definition in the .cc file.
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.h:173: ~VideoDecoderSession()
{}
Put definition in the .cc file.
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.h:194: static int32_t
GetUniqueId();
Should be private.
http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode...
ppapi/examples/video_decoder/video_decoder_session.h:268: void
assertNoGLError();
nit: capitalization: Assert
Everybody: Thanks for a lot of comments! Victoria: You're absolutely right, this is hybrid currently and not very good either as a tester or example. Background for this was that when I started writing the video decoder tests I realized they will rely heavily on the video decoder state and I will need to implement the client interface anyway in the tester and the whole flow as well. So I thought that I'll try to nail two flies with one move: I thought I'll create a helper class on top of the video decoder API that can be used by the tester to validate the API. At the same time this class could be used as starting point for applications that use the video decoder. For the actual example plugin, my intention was to implement really simplistic plugin that uses VideoDecoderSession for the video decoding once finished with the tester. This is currently non-existing. Video tester with VideoDecoderSession is really close to a workable tester, but the example plugin is non-existing atm. My current plan has been to try to get the tester working and that is it. Should I move the session from examples/ to tests/? http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... File ppapi/examples/video_decoder/video_decoder_session.cc (right): http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:31: std::ifstream* file = On 2011/06/02 01:47:02, Victoria Kirst wrote: > scoped_ptr, then delete the deletes below :) I am in impression that relying on anything from Chromium is very discouraged in Pepper world. Once somebody, I'm glad to make the changes to use scoped_ptr wherever it is useful. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:53: int32_t Read(uint8_t* target_mem, uint32_t size) { On 2011/06/02 01:47:02, Victoria Kirst wrote: > It looks like you are doing some decoding to split up bitstream buffers based on > a start code of 0001. I don't understand why this needs to be done here; it > seems to me you should simply be passing raw buffers to the decoder, which will > do the decoding. There is possibility to pass the whole bitstream file at a time. Especially, as we are reading the whole thing to memory at the moment. However, when test files get large, this may prove to be a problem. Thus the TODO above for actually reading data as we go on. All video decoders expect the bitstream as complete units, name and content of the unit varies by the standard. You cannot arbitrarily split the bitstream and expect the video decoder finish the decoding without problems. > At the very least you need write some comments explaining what format you're > expecting the raw data to be in, but again it doesn't make sense to me why the > client needs to look at the bitstream like this. Client needs to provide decoder complete decodable units to produce correct output. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:61: // start code found On 2011/06/02 01:47:02, Victoria Kirst wrote: > Complete sentence + explanation of start code. Done. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:65: // back-up 1 byte. On 2011/06/02 01:47:02, Victoria Kirst wrote: > Complete sentence. Done. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:74: // Last unit. Set the unit_end to point to the last byte. On 2011/06/02 01:47:02, Victoria Kirst wrote: > The last buffer begins _and_ ends with 0001? > In other words the buffers look like: > > buffer[0] = 0001<data> > buffer[1] = 0001<data> > ... > buffer[n] = 0001<data>0001 There's no 0001 at the end. +4 needed because of the lookahead done in while loop above. Btw. I ripped your buffer description to my comments to make them more illustrative. :) Thanks! http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:94: uint8_t* mem_; On 2011/06/02 01:47:02, Victoria Kirst wrote: > scoped_array? At the very least you need to delete mem_ in the destructor; right > now you're leaking. True. Added delete for destructor since I am not sure about scoped_ptr in pepper code. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:104: delete video_source_; On 2011/06/02 01:47:02, Victoria Kirst wrote: > No longer needed w/ scoped_ptr Ditto. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:119: if (read_bytes <= 0) { On 2011/06/02 01:47:02, Victoria Kirst wrote: > Nit: no {} for one-line condition Done. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:138: assert(video_source_ && display_); On 2011/06/02 01:47:02, Victoria Kirst wrote: > Do this check in the constructor. Done. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:140: if (!AllocateInputBuffers()) { On 2011/06/02 01:47:02, Victoria Kirst wrote: > Nit: No {} Done. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:146: if (!video_decoder_) { On 2011/06/02 01:47:02, Victoria Kirst wrote: > Nit: No {} Done. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:159: if (!ReadAndDispatchBitstreamUnit((*it).first)) { On 2011/06/02 01:47:02, Victoria Kirst wrote: > Nit: {} Done. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:196: const std::vector<uint32_t>& buffer_properties) { On 2011/06/01 22:41:31, Victoria Kirst wrote: > On 2011/06/01 11:50:08, vmr1 wrote: > > On 2011/05/31 22:29:07, vjain wrote: > > > What is the purpose of buffer_properties? > > > Will GPU video decoder report the dimensions of the video or should the > > > application/plug-in rely on the parameters it obtained from locally decoding > > SPS > > > and PPS? > > > > Buffer properties will contain width & height and the color format of the > > buffer. They will be determined as key-value pairs as defined in > pp_video_dev.h. > > Actually, this C++ API is not up to date with the C API. > > Ville-Mikko's comment holds for the original design of the API. However, our > approach now is to establish properties such as the output color format in the > Create/Init() phase. Instead of this buffer_properties vector, we request > dimensions and other properties as explicit parameters in ProvidePictureBuffers > -- see ppp_video_decoder_dev.h. C++ API change (http://codereview.chromium.org/7085030/) is just about to land. I will rebase the CL to interface described by Victoria. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:252: void VideoDecoderSession::OnBitstreamBufferProcessed( On 2011/06/02 01:04:52, vjain wrote: > Hi Ville-Mikko, > When the GPU decoder returns the buffer id (of bitstream buffer), would it set > the "size" data field to '0'? > If not, do we plan to add this in the code? I am not sure what are you referring to. Bitstream_buffer_id is the id of the bitstream buffer used by the client when it has dispatched the buffer for decoding. Any changes done by the video decoder are not seen here. This information is only to tell that video decoder has finished processing with a specific buffer. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:289: buffer_if_ = static_cast<const struct PPB_Buffer_Dev*>( On 2011/06/02 01:47:02, Victoria Kirst wrote: > ?? Why is this field being set here? Shouldn't this be set in the constructor? Moved to ctor and added assert for variable. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:291: if (!buffer_if_) { On 2011/06/02 01:47:02, Victoria Kirst wrote: > Nit: no {} Removed http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:300: if (bitstream_buffer.data == 0) { On 2011/06/02 01:47:02, Victoria Kirst wrote: > Nit: no {} Done. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:303: bitstream_buffer.size = 0; On 2011/06/02 01:47:02, Victoria Kirst wrote: > This should be kBitstreamBufferSize with change mentioned below (line 337). This is the amount of valid data in buffer, so I don't think it should be kBitstreamBufferSize. Once it contains data, it should contain size bytes of data. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:330: if (target_mem == NULL) { On 2011/06/02 01:47:02, Victoria Kirst wrote: > Nit: no {} Done. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:334: if (!buffer_if_->Describe(bitstream_buffer.data, &size_in_bytes)) { On 2011/06/02 01:47:02, Victoria Kirst wrote: > Nit: no {} Done. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:337: bool success = video_source_->GetBitstreamUnit(target_mem, size_in_bytes, On 2011/06/02 01:47:02, Victoria Kirst wrote: > It's wasteful to allocate a buffer of kBitstreamBufferSize, then copy data that > may be less than kBitstreamBufferSize into it. > > But it seems like GetBitstreamUnit should always copy bitstream_buffer.size > bytes regardless, so if you fix the GetBitstreamUnit logic above then you no > longer have this problem. Already commented above. You cannot arbitrarily split bitstream anywhere. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:339: if (!success) { On 2011/06/02 01:47:02, Victoria Kirst wrote: > Nit: no {} Done. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:344: bitstream_buffers_[bitstream_buffer_id], On 2011/06/02 01:47:02, Victoria Kirst wrote: > bitstream_buffer? Yep. Also, I changed the bitstream_buffer into reference to bitstream_buffers_[bitstream_buffer_id]. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:349: buffer_if_->Unmap(bitstream_buffer.data); On 2011/06/02 01:47:02, Victoria Kirst wrote: > Unmap deletes the bitstream data. Shouldn't we only do this if > OnBitstreamBufferProcessed? True. Done. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:361: "precision highp float; precision highp int;\n" On 2011/06/02 01:47:02, Victoria Kirst wrote: > I don't believe these precision specifiers are necessary because you are not > using float or int in your shader. I am by no means shader expert, so I'll just take your word for it. :) http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:374: "precision mediump float;\n" On 2011/06/02 01:47:02, Victoria Kirst wrote: > Ditto above. Done. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:392: if (!InitGL(surface_size_.width, surface_size_.height)) { On 2011/06/02 01:47:02, Victoria Kirst wrote: > nit: no {} Done. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:401: PP_GLESBuffer_Dev* picture_buffer) { On 2011/06/02 01:47:02, Victoria Kirst wrote: > Would make sense to also do the initial call to TexImage2D in this function to > actually allocate space on the texture. I don't know GL well, so I will need your help in understanding this. I thought initially that BindTexture would allocate the memory as well. Now it seems that it won't. So, do I need to use TexImage2D to allocate the data and write something to it initially to actually have the memory allocated and prepared for the video decoder? How about when we give the texture ID to EGLImage API to generate target EGL for video decoder? I thought the video decoder will write the output image transparently to the texture without any involvement from the GLES client. After video decoder says that it has finished decoding the video frame to the texture, we simply trigger the GLES pipeline to do it's magic (using DrawArrays) without uploading any extra data into the texture (using TexImage2D). What's wrong in this workflow? http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:425: pp::CompletionCallback completion_callback) { On 2011/06/02 01:47:02, Victoria Kirst wrote: > nit: add space before pp Done. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:435: // Draw the texture. On 2011/06/02 01:47:02, Victoria Kirst wrote: > You never copy data into the texture (TexImage2D/MapTexSubImage2DCHROMIUM), so > you are not drawing anything. See above comment about ProvideGLESPictureBuffer. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:444: if (error != PP_OK) { On 2011/06/02 01:47:02, Victoria Kirst wrote: > nit: no {} Done. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:512: assert(result); On 2011/06/02 01:47:02, Victoria Kirst wrote: > You never use log... change to fail with "log" as error message? In Pepper code? http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:568: 8 * sizeof(kVertices[0]), kVertices, GL_STREAM_DRAW); On 2011/06/02 01:47:02, Victoria Kirst wrote: > Should this be GL_STATIC_DRAW? I think both will work. I took your suggestion. Done. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:583: kTextureCoordsEgl, GL_STREAM_DRAW); On 2011/06/02 01:47:02, Victoria Kirst wrote: > Ditto above. Ditto. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.cc:592: gles2_if_->Enable(context_->pp_resource(), GL_DEPTH_TEST); On 2011/06/02 01:47:02, Victoria Kirst wrote: > What is GL_DEPTH_TEST? I believe it is feature of the fragment pipeline in GPU which tests whether certain fragment is behind some other object. If it is behind, rest of the pipeline will not execute and draw the fragment. I removed it as it seems obsolete in the context of this context. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... File ppapi/examples/video_decoder/video_decoder_session.h (right): http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.h:80: PP_GLESBuffer_Dev* picture_buffer) = 0; On 2011/06/02 01:47:02, Victoria Kirst wrote: > Why does this only provide one picture buffer at a time? Seems like > VideoDecoderSession should just forward its ProvidePictureBuffers command to > this function. Changed the function to output multiple pictures. Just forwarding won't do, because I think it is better to keep the display unrelated to video decoder, which means the Session object should run the callback to decoder. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.h:105: TestVideoSource* video_source_; On 2011/06/02 01:47:02, Victoria Kirst wrote: > scoped_ptr? Probably not on Pepper-side of things? Trying to avoid any dependencies with Chromium code. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.h:172: state_(kCreated) {} On 2011/06/02 01:47:02, Victoria Kirst wrote: > Put definition in the .cc file. Done. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.h:173: ~VideoDecoderSession() {} On 2011/06/02 01:47:02, Victoria Kirst wrote: > Put definition in the .cc file. Done. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.h:194: static int32_t GetUniqueId(); On 2011/06/02 01:47:02, Victoria Kirst wrote: > Should be private. Done. http://codereview.chromium.org/6961018/diff/21001/ppapi/examples/video_decode... ppapi/examples/video_decoder/video_decoder_session.h:268: void assertNoGLError(); On 2011/06/02 01:47:02, Victoria Kirst wrote: > nit: capitalization: Assert Done.
I need guidance on how to handle VideoDecoderSession helper class to finish this CL. Options are (feel free to suggest something else if you have in mind): 1. Revert the VideoDecoderSession (further abstraction on top of Video Decoder API) to be a integral part of TestVideoDecoder and be defined in test_video_decoder.h/cc completely. 2. Move VideoDecoderSession under ppapi/lib and include that as part of the tester to implement the tests. 3. Leave VideoDecoderSession under examples as independent file for future and finish tester referencing this file.
Closing the issue as suggested by Ami.
[putting my email referenced by vmr1 directly into reviewlog since apparently I managed to mess up the original send yesterday] Ville-Mikko, All three options you describe represent a significant amount of work for you, and none are likely to result in immediate significant benefit to our primary customer. Instead of trying to land this CL as a complete entity, I propose leaving it on codereviewer (closed), and vrk/I will use it as a guide/example for extending the gles2 example plugin to do HW decode (which will be much less work since it doesn't have to be generic). Once we have the example plugin decoding video in HW it should be more apparent what pieces would be useful to put into a helper library, and we can pull what is useful from your CL into that, too. Thanks for all your work on this! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
