Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11973010:
AndroidVDA by using Android's MediaCodec API. (Closed)
|
Created:
7 years, 11 months ago by dwkang1 Modified:
6 years, 3 months ago Reviewers:
greggman, Min Qin, Ami GONE FROM CHROMIUM, felipeg, sheu, ycheo (away), piman, qinmin, dssxkk, xhwang CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, apatrick_chromium, feature-media-reviews_chromium.org, wjia(left Chromium), Pawel Osciak, felipeg, googletv-nikita_google.com Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAndroidVDA using Android's MediaCodec API.
This is part of the effort for HW decoding on WebRTC.
This change will not affect the existing media playback because Chrome for Android uses a separate WMP.
Also, VDA is disabled on Chrome for Android. The plan is to enable it after connect VDA to WebRTC.
TODO:
- to make vda unittest work on Android.
@ media_codec_bridge* are written by ycheo@
BUG=170345
TEST=visited the follwing sites after turning on Chrome's media stack
with a separate change.
http://html5example.net/static/html/html5-WebM-VP8-video.html
http://www.ioncannon.net/examples/vp8-webm/demo.html
http://www.dailymotion.com/html5
http://easyhtml5video.com
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=185469
Patch Set 1 #
Total comments: 16
Patch Set 2 : #
Total comments: 195
Patch Set 3 : marking XXX for issues should be resolved before committing. #
Total comments: 103
Patch Set 4 : using flush() for reset(). #
Total comments: 79
Patch Set 5 : following up Min's comments. #Patch Set 6 : apply Ami's comments. #Patch Set 7 : #
Total comments: 13
Patch Set 8 : using "ui/gl/gl_bindings.h" #
Total comments: 15
Patch Set 9 : re-upload #Patch Set 10 : Original Patch from Dongwong #Patch Set 11 : adding save/restore status in copier. #Patch Set 12 : adding save/restore status in copier. #
Total comments: 18
Patch Set 13 : rebasing and better NEOBB(). #
Total comments: 5
Patch Set 14 : using CopyTextureCHROMIUMResourceManager #
Total comments: 14
Patch Set 15 : rebase #Patch Set 16 : disable H264 and follow up gman's comments. #Patch Set 17 : disable H264 and follow up gman's comments. #Patch Set 18 : rebase and patching 12315051 #Patch Set 19 : using GLES2Decoder for restoring #
Total comments: 4
Patch Set 20 : adding android api version checking. #
Total comments: 5
Patch Set 21 : rebased #Patch Set 22 : apply gman's comment #Patch Set 23 : applying ami's comment. #
Total comments: 4
Patch Set 24 : apply xhwang's comment. #
Total comments: 3
Patch Set 25 : apply piman's comment #Patch Set 26 : adding gl.gyp:gl dependancy #Patch Set 27 : reverting last patch set. #Patch Set 28 : fixing android_clang build error. #Messages
Total messages: 65 (0 generated)
https://codereview.chromium.org/11973010/diff/1/content/common/gpu/media/andr... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/11973010/diff/1/content/common/gpu/media/andr... content/common/gpu/media/android_video_decode_accelerator.cc:35: enum { kDequeueInputBufferTimeOutUs = 10 }; Could you leave some comment on why you choose this value? https://codereview.chromium.org/11973010/diff/1/content/common/gpu/media/andr... content/common/gpu/media/android_video_decode_accelerator.cc:113: base::TimeDelta::FromMilliseconds(kDecodePollDelayMs)); Why don't you make the whole TimeDelta as a constant? https://codereview.chromium.org/11973010/diff/1/content/common/gpu/media/andr... content/common/gpu/media/android_video_decode_accelerator.cc:139: return; What can we do for the dequeued input_buf_index? https://codereview.chromium.org/11973010/diff/1/content/common/gpu/media/andr... content/common/gpu/media/android_video_decode_accelerator.cc:179: case -1: // INFO_TRY_AGAIN_LATER Sorry, this should be my work. please define these constants in media_codec_bridge.h. https://codereview.chromium.org/11973010/diff/1/content/common/gpu/media/andr... content/common/gpu/media/android_video_decode_accelerator.cc:181: break; redundant? https://codereview.chromium.org/11973010/diff/1/content/common/gpu/media/andr... content/common/gpu/media/android_video_decode_accelerator.cc:344: media_codec_->Stop(); Why not Release()? https://codereview.chromium.org/11973010/diff/1/content/common/gpu/media/gles... File content/common/gpu/media/gles2_external_texture_copier.cc (right): https://codereview.chromium.org/11973010/diff/1/content/common/gpu/media/gles... content/common/gpu/media/gles2_external_texture_copier.cc:171: glClear( GL_DEPTH_BUFFER_BIT | GL_COLOR_BUFFER_BIT); Remove a space after the opening parenthesis. https://codereview.chromium.org/11973010/diff/1/content/common/gpu/media/gles... content/common/gpu/media/gles2_external_texture_copier.cc:236: memcpy(st_matrix_, transfrom_matrix, sizeof(st_matrix_)); What's the reason to copy transform_matrix to st_matrix_ not to handle it over RenderFrame()?
Thanks for the review. PTAL. https://codereview.chromium.org/11973010/diff/1/content/common/gpu/media/andr... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/11973010/diff/1/content/common/gpu/media/andr... content/common/gpu/media/android_video_decode_accelerator.cc:35: enum { kDequeueInputBufferTimeOutUs = 10 }; On 2013/01/17 08:34:44, ycheo wrote: > Could you leave some comment on why you choose this value? Done. https://codereview.chromium.org/11973010/diff/1/content/common/gpu/media/andr... content/common/gpu/media/android_video_decode_accelerator.cc:113: base::TimeDelta::FromMilliseconds(kDecodePollDelayMs)); On 2013/01/17 08:34:44, ycheo wrote: > Why don't you make the whole TimeDelta as a constant? Done. https://codereview.chromium.org/11973010/diff/1/content/common/gpu/media/andr... content/common/gpu/media/android_video_decode_accelerator.cc:139: return; On 2013/01/17 08:34:44, ycheo wrote: > What can we do for the dequeued input_buf_index? Not much. I've noticed that other vda implementation reporting error in this case and did the same thing here. Thanks for pointing this! https://codereview.chromium.org/11973010/diff/1/content/common/gpu/media/andr... content/common/gpu/media/android_video_decode_accelerator.cc:179: case -1: // INFO_TRY_AGAIN_LATER On 2013/01/17 08:34:44, ycheo wrote: > Sorry, this should be my work. please define these constants in > media_codec_bridge.h. Done. https://codereview.chromium.org/11973010/diff/1/content/common/gpu/media/andr... content/common/gpu/media/android_video_decode_accelerator.cc:181: break; On 2013/01/17 08:34:44, ycheo wrote: > redundant? Removed. https://codereview.chromium.org/11973010/diff/1/content/common/gpu/media/andr... content/common/gpu/media/android_video_decode_accelerator.cc:344: media_codec_->Stop(); On 2013/01/17 08:34:44, ycheo wrote: > Why not Release()? Done. https://codereview.chromium.org/11973010/diff/1/content/common/gpu/media/gles... File content/common/gpu/media/gles2_external_texture_copier.cc (right): https://codereview.chromium.org/11973010/diff/1/content/common/gpu/media/gles... content/common/gpu/media/gles2_external_texture_copier.cc:171: glClear( GL_DEPTH_BUFFER_BIT | GL_COLOR_BUFFER_BIT); On 2013/01/17 08:34:44, ycheo wrote: > Remove a space after the opening parenthesis. Done. https://codereview.chromium.org/11973010/diff/1/content/common/gpu/media/gles... content/common/gpu/media/gles2_external_texture_copier.cc:236: memcpy(st_matrix_, transfrom_matrix, sizeof(st_matrix_)); On 2013/01/17 08:34:44, ycheo wrote: > What's the reason to copy transform_matrix to st_matrix_ not to handle it over > RenderFrame()? Because I did this in a hurry? ;-) Fixed.
Hi reviewers, As noted in the description, this change itself will not change any feature on Chrome for Android. To see it works, please apply the following patches for enabling Chrome's media stack and VDA. https://codereview.chromium.org/11859023/ https://codereview.chromium.org/11943010/ Thanks!
Exciting times! https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:30: #define LOG_LINE() VLOG(1) << __FUNCTION__ drop before submitting? https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:37: // timeout value may improve performance. See my comment on the bridge class; I hope you can drop these. https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:53: surface_texture_id_(0), -1 to be more obvious? https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:57: height_(0), should be able to drop once using gfx::Size (which has a no-arg zero-out ctor) https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:77: }else { missing space https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:80: } Once you pass profile to ConfigureMediaCodec below, I'd simply replace this if/else chain with: if (profile >= media::VP8PROFILE_MAX) return false; https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:82: if (media_codec_ == NULL) { How could this *not* be NULL? https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:87: glGenTextures(1, &surface_texture_id_); Please make sure this file follows the (brand-new) scheme for GL access: http://crbug.com/169433 https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:106: &AndroidVideoDecodeAccelerator::DoDecode, base::Unretained(this))); Per below, drop. https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:116: DequeueOutput(); Shouldn't you drain the output first, to free up decoder resources and possibly increase parallelism? https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:122: &AndroidVideoDecodeAccelerator::DoDecode, base::Unretained(this)), Why is Unretained(this) safe here (and also at l.106 above)? (I believe it is not) https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:123: kDecodePollDelay); Oh my goodness... I didn't realize MediaCodec required polling... Can you measure the cost of the poll in the do-nothing case? Assuming there's no way around polling, I think you want to not queue a task unless there is pending input or output,. https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:127: if (!pending_bitstream_buffers_.empty()) { if you reverse the test you can early-return and de-indent the rest of the function, and make it clearer what's going on. https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:163: client_->NotifyEndOfBitstreamBuffer(bitstream_buffer.id()); This is wrong; you should only NotifyEndOfBitstreamBuffer when the contents of the buffer are known to have been decoded. In other words, it's not OK to call PictureReady with a bitstreambufferid of a bitstreambuffer for which you've already NEOBB'd. https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:170: DLOG(INFO) << "Picture buffers are not ready."; drop https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:173: if (!picture_map_.empty() && free_picture_ids_.empty()) { Drop the picture_map_.empty() clause? SendCurrentSurfaceToBuffer assumes free_pictures_id_ is non-empty. https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:178: int32 output_offset = 0; this and the other params below don't seem to need the output_ prefix. https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:178: int32 output_offset = 0; Declare vars at first use unless there's a reason not to (need to survive a scope). I think here there's no reason not to. https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:181: int64 timestamp = 0; bitstream_buffer_id https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:194: DLOG(INFO) << "Output size: " << width_ << "x" << height_; Drop these two lines https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:202: GL_TEXTURE_2D); If you requested a different texture target could you avoid the copy? (what happened to our conversation about avoiding texture copies by swapping textures out from under surfaces?) https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:204: // TODO(dwkang): support the dynamic resolution change. set state to ERROR and NotifyError? https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:220: current_bitstream_id_ = static_cast<int32>(timestamp); This doesn't need to be a class member; you can just as well pass it to SCSTC() directly. https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:237: LOG(ERROR) << "Failed to make this decoder's GL context current."; Shouldn't this set the state to ERROR and also NotifyError to the client? I think you want a macro like https://code.google.com/searchframe#OAMlx_jo-ck/src/content/common/gpu/media/... https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:251: void AndroidVideoDecodeAccelerator::CopyCurrentFrameToPictureBuffer( single call-site + simple impl == inline into call-site? https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:283: } Add: if (picture_map_.size() != kNumPictureBuffers) { NotifyError(...); state = ERROR; return; } to guard against compromised renderers feeding us garbage, repeated ids, etc. https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:327: // phase. Here, we set 1080p by default. This is a mistake that is likely to hide bugs, IMO. Better to set a width/height of 0 and handle the "format change" when dequeuing outputs. https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:330: content::ReleaseSurface(j_surface.obj()); How does this work? Didn't you just ask MediaCodec to render output to this surface that you're freeing here? https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:347: client_->NotifyEndOfBitstreamBuffer(bitstream_buffer.id()); I don't think the VDA interface requires this (and the other impls don't do it). So this while loop can be replaced with: pending_bitstream_buffers_.clear(); https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:352: media_codec_->Release(); Why the Stop/Release/ConfigureMediaCodec dance? https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:357: client_->NotifyResetDone(); For complicated reasons, it's not OK to make client callbacks on the same stack that the client calls us. Always PostTask callbacks unless you're guaranteed not to be in this situation. https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.cc:364: } This method needs to delete this; or else you have a leak (look at how this is called from GpuVideoDecodeAccelerator). https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... File content/common/gpu/media/android_video_decode_accelerator.h (right): https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.h:11: #include <set> unused https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.h:13: #include <utility> unused? https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.h:18: #include "base/message_loop.h" replace with fwd-declaration https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.h:19: #include "base/shared_memory.h" unnecessary (please audit the rest of the #includes) https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.h:32: typedef std::map<int32, media::PictureBuffer> PictureMap; Only used for a private member; move into the private section of the class. https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.h:44: virtual ~AndroidVideoDecodeAccelerator(); dtor should be private since only Destroy should be deleting. (I guess the other VDA impl's need this change too; I'll probably send a separate CL for that) https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.h:57: enum Codec { You can drop this enum and the codec_ parameter by simply passing the profile param from Initialize to ConfigureMediaCodec(). https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.h:67: static const base::TimeDelta kDecodePollDelay; I don't know what this is used for yet, but I suspect I'm not going to be happy when I find out... ;) https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.h:69: void SendCurrentSurfaceToClient(); You need to document everything that's not obvious, which is pretty much all the methods in this class... https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.h:81: // NOTE: all calls to this object *MUST* be executed in message_loop_. This is the first time there's an implication that perhaps more than one thread will be touching this class. Is that true? If it is, you need a class-level comment explaining the threading characteristics of this class. If it is not true, then I suspect this comment can be dropped, and uses of message_loop_ replaced with MessageLoop::current() (and drop the message_loop_ member). To enforce single-threadedness, use a ThreadChecker. https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.h:86: // These members are only used during Initialization. I think this comment refers to just one member, so s/members/member/ but hopefully both the comment and the member can be dropped. https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.h:95: bool picturebuffer_requested_; s/buffer/buffers/ ? https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.h:98: BitstreamBufferList pending_bitstream_buffers_; pending where? At the decoder or awaiting submission to the decoder (e.g. b/c a Decode() arrived during a Reset())? Comments comments comments :) https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.h:100: int32 color_format_; Can this not be a more specifically-typed member? Document the possibly-held values, or point at something that documents (an enumeration, etc). https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.h:100: int32 color_format_; I believe this and the next two members (width/height) are only used in a single function, so can be local vars there instead of class members. https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/a... content/common/gpu/media/android_video_decode_accelerator.h:102: int32 height_; Replace width/height with gfx::Size https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/g... File content/common/gpu/media/gles2_external_texture_copier.cc (right): https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/g... content/common/gpu/media/gles2_external_texture_copier.cc:6: Skipping reviewing this file since I hope you can drop it :) https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/g... File content/common/gpu/media/gles2_external_texture_copier.h (right): https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/g... content/common/gpu/media/gles2_external_texture_copier.h:17: class Gles2ExternalTextureCopier { Can this class be more or less replaced with a call to copyTextureCHROMIUM ? E.g. something like: https://code.google.com/searchframe#OAMlx_jo-ck/src/content/renderer/media/re... https://codereview.chromium.org/11973010/diff/6001/content/common/gpu/media/g... content/common/gpu/media/gles2_external_texture_copier.h:26: GLuint destination_texture_id, GLenum destination_target); Drop the _target params as they are specified constant in the class comment. https://codereview.chromium.org/11973010/diff/6001/content/content_tests.gypi File content/content_tests.gypi (right): https://codereview.chromium.org/11973010/diff/6001/content/content_tests.gypi... content/content_tests.gypi:827: # TODO(dwkang): make video_decode_accelerator_unittest work on Android. FWIW you should probably get this done before landing this CL. https://codereview.chromium.org/11973010/diff/6001/media/base/android/MediaCo... File media/base/android/MediaCodec_jni.h (right): https://codereview.chromium.org/11973010/diff/6001/media/base/android/MediaCo... media/base/android/MediaCodec_jni.h:5: // This file is autogenerated by Is this in fact an unmodified copy of that generated file? (i.e. do I have to review it?) https://codereview.chromium.org/11973010/diff/6001/media/base/android/MediaCo... media/base/android/MediaCodec_jni.h:8: // android/media/MediaCodec You plan to solve the autogen problem before landing this CL, right? https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... File media/base/android/media_codec_bridge.cc (right): https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.cc:26: static jclass g_MediaCodecBufferInfo_clazz = NULL; Why not move this into BI_FI below, since it's never accessed elsewhere? https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.cc:28: struct BufferInfo_FieldIds { I believe you can drastically simplify the code around this stuff: static void GetBufferInfo(JNIEnv* env, jobject buffer_info, int* offset, int* size, int64* presentation_time, int* flags) { static ScopedJavaLocalRef<jclass> clazz = base::android::GetClass(env, "android/media/MediaCodec$BufferInfo"); static jfieldID offset_id = env->GetFieldID(g_MediaCodecBufferInfo_clazz, "offset", "I"); ... the other 3 field id finders *offset = static_cast<int>(env->GetIntField(buffer_info, offset_id); ... the other 3 field getters } Done. Those 12 lines (plus wrapping for 80-cols) should let you drop l.24-51, and the call-site becomes much simpler. https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.cc:41: static BufferInfo_FieldIds* g_field_ids; s/;/ = NULL;/ https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.cc:50: return true; What if any of the lookups fail? Don't you want to do something like env->ExceptionCheck()? (probably there's a base/android wrapper for that, I haven't looked yet) https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.cc:74: JNI_MediaFormat::RegisterNativesImpl(env); These 3 methods can return false; shouldn't you handle that? https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.cc:116: DCHECK(!j_format.is_null()); Again, I worry about a free-text mime-type causing a crash. https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.cc:136: ConvertUTF8ToJavaString(env, "encoder-delay"); Where does this string come from? I don't see it documented in Media{Format,Codec}'s javadocs. Ditto for the string literal keys elsewhere in this file. https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.cc:156: } There's an aweful lot of copy/pasta above; why not use a helper function? https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.cc:161: env, j_format.obj(), j_is_adts_string.obj(), static_cast<jint>(1)); Why is this always true? https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.cc:168: const std::string& mime, int width, int height, prefer to use gfx::Size instead of width/height params to avoid mixing up the order (and reduce arities/increase readability) https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.cc:177: env, j_mime.obj(), static_cast<jint>(width), static_cast<jint>(height))); 80-cols https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.cc:177: env, j_mime.obj(), static_cast<jint>(width), static_cast<jint>(height))); Is it really necessary to static_cast an int to a jint?? https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.cc:200: void MediaCodecBridge::Start() { Why not make all configuration (audio/video) a single call and then always Start() at the end of it? https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.cc:221: void MediaCodecBridge::Release() { Hopefully you can drop this method in favor of the dtor. https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.cc:312: if (j_input_buffers_.is_null()) return -1; I don't see this in the javadoc. https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.cc:324: if (j_output_buffers_.is_null()) return -1; again, javadoc doesn't support this. https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.cc:338: static_cast<uint8*>(env->GetDirectBufferAddress(j_buffer.obj())); What guarantees that the bytebuffers MediaCodec returns are "direct"? https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.cc:339: memcpy(direct_buffer, data, size); what if |size| is too big? https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.cc:339: memcpy(direct_buffer, data, size); Is it not necessary to set the bytebuffer's mark/position/limit? https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.cc:351: static_cast<uint8*>(env->GetDirectBufferAddress(j_buffer.obj())); ditto what if not direct? https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.cc:366: memcpy(data1, direct_buffer + size0, size1); What guarantees that the planes are mashed together like this, without, e.g. padding to a word or power-of-two boundary? https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... File media/base/android/media_codec_bridge.h (right): https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.h:13: //#include "base/memory/ref_counted.h" remove prev 2 lines https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.h:15: // This class serves as a bridge for native code to call java functions inside Move down to immediately precede class declaration https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.h:35: explicit MediaCodecBridge(const std::string& type); s/type/mime_type/ https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.h:35: explicit MediaCodecBridge(const std::string& type); It seems wrong to me for this to be a ctor and not a static factory method or an Init method, because the type param is free-form. What happens if I pass this "foobar" as the type? Will the DCHECK in the .cc trigger? Or do I get back a non-functioning MediaCodec? (the javadoc isn't particularly helpful on this count :( ) https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.h:39: void ConfigureAudio(const std::string& mime, int sample_rate, s/mime/mime_type/ https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.h:40: int channel_count, const uint8* csd0, int csd0_size, const uint8* csd1, What's up with these csd0/1 params? (why exactly 2 of them, why not use a vector<uint8> or a std::string to avoid having to specify ptr/length separately, etc) https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.h:44: const uint8* csd0, int csd0_size, const uint8* csd1, int csd1_size, Ditto csd questions, and in fact I think you should just drop them entirely, as the VDA should pump them in as part of the bitstream, anyway. https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.h:48: void Flush(); I think MediaCodec.flush() is something media:: code refers to as Reset(). Please document what this does (drops all queued inputs on the floor, ready to start decoding at a new seek point?) https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.h:49: void Stop(); Stop() is irreversible in most media:: code. Please document. https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.h:50: void Release(); Again, please document what this does. Why would someone call this instead of simply deleting the object? Can a Release()'d MediaCodecBridge object ever be used again for any purpose? https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.h:56: int DequeueInputBuffer(int64 timeout_us); document API (I'll stop commenting on this explicitly; please do all the rest, too) https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.h:56: int DequeueInputBuffer(int64 timeout_us); I question the value of providing a "waiting" API at this layer (in other words, I think you want to omit this param and instead always pass 0 to the Java layer). Does the 10us value you use in AVDA behave any differently from a value of 0? https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.h:67: void GetFromOutputBuffer(int index, Neither of the GetFromOutputBuffer methods is called; drop or annotate as not-to-be-used for decode (where we use Surfaces)? https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.h:80: base::android::ScopedJavaGlobalRef<jbyteArray> j_byte_array_; Unused (both prev lines)? https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... File media/base/android/media_codec_bridge_unittest.cc (right): https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge_unittest.cc:93: class MediaCodecBridgeTest : public testing::Test { This fixture adds a bunch of boilerplate but little value. E.g. if you deleted it, the first test would simply turn into: TEST(MediaCodecBridgeTest, Initialize) { MediaCodecBridge media_codec("audio/vorbis"); } https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge_unittest.cc:112: Initialize("audio/vorbis"); indent here and below should be +2 for test bodies (not +4 as you have it). https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge_unittest.cc:125: int input_buf_index; Please declare variables as close to first use as possible (usually at first assignment). https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge_unittest.cc:140: input_buf_index = media_codec_->DequeueInputBuffer(25000); s/25000/0/ here and below. https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge_unittest.cc:155: case MediaCodecBridge::INFO_TRY_AGAIN_LATER: If you take my suggestion to remove the arbitrary timeout, you can drop this. https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge_unittest.cc:158: case MediaCodecBridge::INFO_OUTPUT_FORMAT_CHANGED: Shouldn't this FAIL() the test? https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge_unittest.cc:161: case MediaCodecBridge::INFO_OUTPUT_BUFFERS_CHANGED: Since this is the only expected result, ASSERT it and drop the switch. https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge_unittest.cc:166: if (!(flag & 4)) { s/4/BUFFER_FLAG_END_OF_STREAM/ https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge_unittest.cc:169: } This test would fail to fail if the decoder simply returned EOS immediately. https://codereview.chromium.org/11973010/diff/6001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/11973010/diff/6001/media/media.gyp#newcode805 media/media.gyp:805: 'base/android/media_codec_bridge_unittest.cc', I think this can go in the main sources list (platform-specific paths get automatically omitted from other platform builds; see http://src.chromium.org/viewvc/chrome/trunk/src/build/filename_rules.gypi?vie... )
https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... File media/base/android/media_codec_bridge.cc (right): https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.cc:338: static_cast<uint8*>(env->GetDirectBufferAddress(j_buffer.obj())); On 2013/01/23 01:32:32, Ami Fischman wrote: > What guarantees that the bytebuffers MediaCodec returns are "direct"? Actually, we refereed the underlying implementation. https://cs.corp.google.com/#googletv/frameworks/base/media/jni/android_media_... Do you think that we should check if the buffer is direct? https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.cc:339: memcpy(direct_buffer, data, size); On 2013/01/23 01:32:32, Ami Fischman wrote: > Is it not necessary to set the bytebuffer's mark/position/limit? Yes, the underlying implementation doesn't care about them. That's why the original queueInputBuffer() has separate offset and size fields. http://developer.android.com/reference/android/media/MediaCodec.html#queueInp..., int, int, long, int)
https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... File media/base/android/media_codec_bridge.cc (right): https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.cc:338: static_cast<uint8*>(env->GetDirectBufferAddress(j_buffer.obj())); On 2013/01/27 07:34:48, ycheo wrote: > On 2013/01/23 01:32:32, Ami Fischman wrote: > > What guarantees that the bytebuffers MediaCodec returns are "direct"? > Actually, we refereed the underlying implementation. > https://cs.corp.google.com/#googletv/frameworks/base/media/jni/android_media_... > > Do you think that we should check if the buffer is direct? If all known impls are direct then probably not worth adding in code that'll never run. Are you in a position to get the MediaCodec spec changed to specify that returned buffers are direct (at least for future SDK releases)?
https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... File media/base/android/media_codec_bridge.cc (right): https://codereview.chromium.org/11973010/diff/6001/media/base/android/media_c... media/base/android/media_codec_bridge.cc:338: static_cast<uint8*>(env->GetDirectBufferAddress(j_buffer.obj())); On 2013/01/27 17:50:42, Ami Fischman wrote: > On 2013/01/27 07:34:48, ycheo wrote: > > On 2013/01/23 01:32:32, Ami Fischman wrote: > > > What guarantees that the bytebuffers MediaCodec returns are "direct"? > > Actually, we refereed the underlying implementation. > > > https://cs.corp.google.com/#googletv/frameworks/base/media/jni/android_media_... > > > > Do you think that we should check if the buffer is direct? > > If all known impls are direct then probably not worth adding in code that'll > never run. > Are you in a position to get the MediaCodec spec changed to specify that > returned buffers are direct (at least for future SDK releases)? We can ask them, but I don't expect that they try to change this actively.
Thanks for the review and sorry for the late response. I was interrupted by other tasks. Please note that I've marked XXX for the issues that will be resolved before submitting. Thanks! https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:30: #define LOG_LINE() VLOG(1) << __FUNCTION__ On 2013/01/23 01:32:32, Ami Fischman wrote: > drop before submitting? Marked with XXX. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:37: // timeout value may improve performance. On 2013/01/23 01:32:32, Ami Fischman wrote: > See my comment on the bridge class; I hope you can drop these. Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:53: surface_texture_id_(0), On 2013/01/23 01:32:32, Ami Fischman wrote: > -1 to be more obvious? Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:57: height_(0), On 2013/01/23 01:32:32, Ami Fischman wrote: > should be able to drop once using gfx::Size (which has a no-arg zero-out ctor) Removed. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:77: }else { On 2013/01/23 01:32:32, Ami Fischman wrote: > missing space Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:80: } On 2013/01/23 01:32:32, Ami Fischman wrote: > Once you pass profile to ConfigureMediaCodec below, I'd simply replace this > if/else chain with: > if (profile >= media::VP8PROFILE_MAX) > return false; codec_ is used when re-configuring MediaCodec. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:82: if (media_codec_ == NULL) { On 2013/01/23 01:32:32, Ami Fischman wrote: > How could this *not* be NULL? Changed to DCHECK. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:87: glGenTextures(1, &surface_texture_id_); On 2013/01/23 01:32:32, Ami Fischman wrote: > Please make sure this file follows the (brand-new) scheme for GL access: > http://crbug.com/169433 Let me address this issue once we finalize how we handle texture copy. Will be marked with XXX and will be addressed before submitting. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:106: &AndroidVideoDecodeAccelerator::DoDecode, base::Unretained(this))); On 2013/01/23 01:32:32, Ami Fischman wrote: > Per below, drop. Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:116: DequeueOutput(); On 2013/01/23 01:32:32, Ami Fischman wrote: > Shouldn't you drain the output first, to free up decoder resources and possibly > increase parallelism? Makes sense. Changed. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:122: &AndroidVideoDecodeAccelerator::DoDecode, base::Unretained(this)), On 2013/01/23 01:32:32, Ami Fischman wrote: > Why is Unretained(this) safe here (and also at l.106 above)? > (I believe it is not) You are right. Changed to base::AsWeakPtr(). https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:123: kDecodePollDelay); On 2013/01/23 01:32:32, Ami Fischman wrote: > Oh my goodness... I didn't realize MediaCodec required polling... > Can you measure the cost of the poll in the do-nothing case? with 10ms, cost was about 3% of cpu rate. > > Assuming there's no way around polling, I think you want to not queue a task > unless there is pending input or output,. Good idea. Thanks! https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:127: if (!pending_bitstream_buffers_.empty()) { On 2013/01/23 01:32:32, Ami Fischman wrote: > if you reverse the test you can early-return and de-indent the rest of the > function, and make it clearer what's going on. Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:163: client_->NotifyEndOfBitstreamBuffer(bitstream_buffer.id()); On 2013/01/23 01:32:32, Ami Fischman wrote: > This is wrong; you should only NotifyEndOfBitstreamBuffer when the contents of > the buffer are known to have been decoded. > In other words, it's not OK to call PictureReady with a bitstreambufferid of a > bitstreambuffer for which you've already NEOBB'd. When I wrote this code, I referred OVDA code. In OmxVideoDecodeAccelerator::EmptyBufferDoneTask(), which is a callback for input feeding, it calls NEOBB(). Do you think OVDA's implementation is wrong also? https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:170: DLOG(INFO) << "Picture buffers are not ready."; On 2013/01/23 01:32:32, Ami Fischman wrote: > drop Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:173: if (!picture_map_.empty() && free_picture_ids_.empty()) { On 2013/01/23 01:32:32, Ami Fischman wrote: > Drop the picture_map_.empty() clause? > SendCurrentSurfaceToBuffer assumes free_pictures_id_ is non-empty. Before picturebuffers is requested, free_picture_ids_ will be always empty. So, we need !picture_map_.empty() clause. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:178: int32 output_offset = 0; On 2013/01/23 01:32:32, Ami Fischman wrote: > this and the other params below don't seem to need the output_ prefix. Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:178: int32 output_offset = 0; On 2013/01/23 01:32:32, Ami Fischman wrote: > Declare vars at first use unless there's a reason not to (need to survive a > scope). I think here there's no reason not to. Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:181: int64 timestamp = 0; On 2013/01/23 01:32:32, Ami Fischman wrote: > bitstream_buffer_id Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:194: DLOG(INFO) << "Output size: " << width_ << "x" << height_; On 2013/01/23 01:32:32, Ami Fischman wrote: > Drop these two lines Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:202: GL_TEXTURE_2D); On 2013/01/23 01:32:32, Ami Fischman wrote: > If you requested a different texture target could you avoid the copy? > > (what happened to our conversation about avoiding texture copies by swapping > textures out from under surfaces?) Actually, using SurfaceTexure.attachToGLContext() and detachFromGLContext() was the first thing I tried, but it failed because of the following two reasons. 1. Once we call detachFrameGLContext(), it deletes the texture previous attached. You can check the implementation of SurfaceTexture::detachFromContext() in frameworks/native/libs/gui/SurfaceTexture.cpp If you have access to the Android code. 2. This may not be a blocker for using the approach, but SurfaceTexture requires us to apply a transform matrix when we show the texture. And according to the documentation it may vary frame to frame. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:204: // TODO(dwkang): support the dynamic resolution change. On 2013/01/23 01:32:32, Ami Fischman wrote: > set state to ERROR and NotifyError? Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:220: current_bitstream_id_ = static_cast<int32>(timestamp); On 2013/01/23 01:32:32, Ami Fischman wrote: > This doesn't need to be a class member; you can just as well pass it to SCSTC() > directly. Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:237: LOG(ERROR) << "Failed to make this decoder's GL context current."; On 2013/01/23 01:32:32, Ami Fischman wrote: > Shouldn't this set the state to ERROR and also NotifyError to the client? I > think you want a macro like > https://code.google.com/searchframe#OAMlx_jo-ck/src/content/common/gpu/media/... Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:251: void AndroidVideoDecodeAccelerator::CopyCurrentFrameToPictureBuffer( On 2013/01/23 01:32:32, Ami Fischman wrote: > single call-site + simple impl == inline into call-site? I think it depends on how we will define "simple", but I don't have strong objection. Merged with SCSTC(). https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:283: } On 2013/01/23 01:32:32, Ami Fischman wrote: > Add: > if (picture_map_.size() != kNumPictureBuffers) { > NotifyError(...); > state = ERROR; > return; > } > to guard against compromised renderers feeding us garbage, repeated ids, etc. Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:327: // phase. Here, we set 1080p by default. On 2013/01/23 01:32:32, Ami Fischman wrote: > This is a mistake that is likely to hide bugs, IMO. > Better to set a width/height of 0 and handle the "format change" when dequeuing > outputs. Actually, that was my first try, but it lead to Exception from MediaCodec. I think options for avoiding this ugly hard-coded values is 1. pass the container indicated resolution to vda. 2. start with 1080p and wait until the real resolution and start from the beginning. What do you think? https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:330: content::ReleaseSurface(j_surface.obj()); On 2013/01/23 01:32:32, Ami Fischman wrote: > How does this work? Didn't you just ask MediaCodec to render output to this > surface that you're freeing here? If Surface is not registered to the windows manager, like this case, Surface is just a mean to pass the texture to MediaCodec. other example: https://cs.corp.google.com/#chrome/src/content/browser/android/surface_textur... https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:347: client_->NotifyEndOfBitstreamBuffer(bitstream_buffer.id()); On 2013/01/23 01:32:32, Ami Fischman wrote: > I don't think the VDA interface requires this (and the other impls don't do it). > So this while loop can be replaced with: > pending_bitstream_buffers_.clear(); Actually, I referred OVDA code when I wrote this code. In OVDA, on Reset(), it flushes the ports. According to the openmax spec 1.2 (3.2.2.4 OMX_CommandFlush), this flushing command will make component return unprocessed buffers to OVDA by calling EmptyBufferDoneTask() and it will call NEOBB. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:352: media_codec_->Release(); On 2013/01/23 01:32:32, Ami Fischman wrote: > Why the Stop/Release/ConfigureMediaCodec dance? It's a recommended way to reset MediaCodec. FWIW, I am not a big fan of MediaCodec. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:357: client_->NotifyResetDone(); On 2013/01/23 01:32:32, Ami Fischman wrote: > For complicated reasons, it's not OK to make client callbacks on the same stack > that the client calls us. Always PostTask callbacks unless you're guaranteed > not to be in this situation. Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:364: } On 2013/01/23 01:32:32, Ami Fischman wrote: > This method needs to > delete this; > or else you have a leak (look at how this is called from > GpuVideoDecodeAccelerator). Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... File content/common/gpu/media/android_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.h:11: #include <set> On 2013/01/23 01:32:32, Ami Fischman wrote: > unused Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.h:13: #include <utility> On 2013/01/23 01:32:32, Ami Fischman wrote: > unused? Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.h:18: #include "base/message_loop.h" On 2013/01/23 01:32:32, Ami Fischman wrote: > replace with fwd-declaration Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.h:19: #include "base/shared_memory.h" On 2013/01/23 01:32:32, Ami Fischman wrote: > unnecessary (please audit the rest of the #includes) Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.h:32: typedef std::map<int32, media::PictureBuffer> PictureMap; On 2013/01/23 01:32:32, Ami Fischman wrote: > Only used for a private member; move into the private section of the class. Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.h:44: virtual ~AndroidVideoDecodeAccelerator(); On 2013/01/23 01:32:32, Ami Fischman wrote: > dtor should be private since only Destroy should be deleting. > (I guess the other VDA impl's need this change too; I'll probably send a > separate CL for that) Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.h:57: enum Codec { On 2013/01/23 01:32:32, Ami Fischman wrote: > You can drop this enum and the codec_ parameter by simply passing the profile > param from Initialize to ConfigureMediaCodec(). Actually, codec_ is needed when we reconfigure MediaCodec. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.h:67: static const base::TimeDelta kDecodePollDelay; On 2013/01/23 01:32:32, Ami Fischman wrote: > I don't know what this is used for yet, but I suspect I'm not going to be happy > when I find out... ;) As you found, I am afraid that I have to say MediaCodec requires it. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.h:69: void SendCurrentSurfaceToClient(); On 2013/01/23 01:32:32, Ami Fischman wrote: > You need to document everything that's not obvious, which is pretty much all the > methods in this class... Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.h:81: // NOTE: all calls to this object *MUST* be executed in message_loop_. On 2013/01/23 01:32:32, Ami Fischman wrote: > This is the first time there's an implication that perhaps more than one thread > will be touching this class. Is that true? If it is, you need a class-level > comment explaining the threading characteristics of this class. > If it is not true, then I suspect this comment can be dropped, and uses of > message_loop_ replaced with MessageLoop::current() (and drop the message_loop_ > member). To enforce single-threadedness, use a ThreadChecker. That is not true. I should have deleted this comments before uploading. Will add ThreadChecker. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.h:86: // These members are only used during Initialization. On 2013/01/23 01:32:32, Ami Fischman wrote: > I think this comment refers to just one member, so s/members/member/ but > hopefully both the comment and the member can be dropped. Ditto. My bad. I should have removed this also. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.h:95: bool picturebuffer_requested_; On 2013/01/23 01:32:32, Ami Fischman wrote: > s/buffer/buffers/ ? Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.h:98: BitstreamBufferList pending_bitstream_buffers_; On 2013/01/23 01:32:32, Ami Fischman wrote: > pending where? At the decoder or awaiting submission to the decoder (e.g. b/c a > Decode() arrived during a Reset())? > Comments comments comments :) Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.h:100: int32 color_format_; On 2013/01/23 01:32:32, Ami Fischman wrote: > Can this not be a more specifically-typed member? > Document the possibly-held values, or point at something that documents (an > enumeration, etc). Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.h:100: int32 color_format_; On 2013/01/23 01:32:32, Ami Fischman wrote: > Can this not be a more specifically-typed member? > Document the possibly-held values, or point at something that documents (an > enumeration, etc). Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.h:102: int32 height_; On 2013/01/23 01:32:32, Ami Fischman wrote: > Replace width/height with gfx::Size Removed. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... File content/common/gpu/media/gles2_external_texture_copier.h (right): https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/gles2_external_texture_copier.h:17: class Gles2ExternalTextureCopier { On 2013/01/23 01:32:32, Ami Fischman wrote: > Can this class be more or less replaced with a call to copyTextureCHROMIUM ? > E.g. something like: > https://code.google.com/searchframe#OAMlx_jo-ck/src/content/renderer/media/re... IIRC, it looks like a call which is supposed to be called in the rendering thread. Is there any way to use it in gpu process? Could you elaborate your idea? https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/gles2_external_texture_copier.h:26: GLuint destination_texture_id, GLenum destination_target); On 2013/01/23 01:32:32, Ami Fischman wrote: > Drop the _target params as they are specified constant in the class comment. Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/content/content_tes... File content/content_tests.gypi (right): https://chromiumcodereview.appspot.com/11973010/diff/6001/content/content_tes... content/content_tests.gypi:827: # TODO(dwkang): make video_decode_accelerator_unittest work on Android. On 2013/01/23 01:32:32, Ami Fischman wrote: > FWIW you should probably get this done before landing this CL. Sure, actually, Fellipe(from Clank team) suggested me to help this issue. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... File media/base/android/MediaCodec_jni.h (right): https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/MediaCodec_jni.h:5: // This file is autogenerated by On 2013/01/23 01:32:32, Ami Fischman wrote: > Is this in fact an unmodified copy of that generated file? (i.e. do I have to > review it?) You shouldn't. =) https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/MediaCodec_jni.h:8: // android/media/MediaCodec On 2013/01/23 01:32:32, Ami Fischman wrote: > You plan to solve the autogen problem before landing this CL, right? Fixed the bug in jni_generator. https://codereview.chromium.org/12094008/ https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... File media/base/android/media_codec_bridge.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge.cc:28: struct BufferInfo_FieldIds { On 2013/01/23 01:32:32, Ami Fischman wrote: > I believe you can drastically simplify the code around this stuff: > > static void GetBufferInfo(JNIEnv* env, jobject buffer_info, int* offset, int* > size, int64* presentation_time, int* flags) { > static ScopedJavaLocalRef<jclass> clazz = base::android::GetClass(env, > "android/media/MediaCodec$BufferInfo"); > static jfieldID offset_id = env->GetFieldID(g_MediaCodecBufferInfo_clazz, > "offset", "I"); > ... the other 3 field id finders > *offset = static_cast<int>(env->GetIntField(buffer_info, offset_id); > ... the other 3 field getters > } > > Done. > > Those 12 lines (plus wrapping for 80-cols) should let you drop l.24-51, and the > call-site becomes much simpler. Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge.cc:41: static BufferInfo_FieldIds* g_field_ids; On 2013/01/23 01:32:32, Ami Fischman wrote: > s/;/ = NULL;/ Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge.cc:50: return true; On 2013/01/23 01:32:32, Ami Fischman wrote: > What if any of the lookups fail? Don't you want to do something like > env->ExceptionCheck()? (probably there's a base/android wrapper for that, I > haven't looked yet) base::android::CheckException(env) is added. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge.cc:74: JNI_MediaFormat::RegisterNativesImpl(env); On 2013/01/23 01:32:32, Ami Fischman wrote: > These 3 methods can return false; shouldn't you handle that? Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge.cc:116: DCHECK(!j_format.is_null()); On 2013/01/23 01:32:32, Ami Fischman wrote: > Again, I worry about a free-text mime-type causing a crash. CodecToMimeType() is introduced for type checking. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge.cc:136: ConvertUTF8ToJavaString(env, "encoder-delay"); On 2013/01/23 01:32:32, Ami Fischman wrote: > Where does this string come from? I don't see it documented in > Media{Format,Codec}'s javadocs. > Ditto for the string literal keys elsewhere in this file. Removed. Will add when these are needed. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge.cc:156: } On 2013/01/23 01:32:32, Ami Fischman wrote: > There's an aweful lot of copy/pasta above; why not use a helper function? Removed. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge.cc:161: env, j_format.obj(), j_is_adts_string.obj(), static_cast<jint>(1)); On 2013/01/23 01:32:32, Ami Fischman wrote: > Why is this always true? I believe this is added while testing AAC codec and not properly handled later. Removed and will be added when needed. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge.cc:168: const std::string& mime, int width, int height, On 2013/01/23 01:32:32, Ami Fischman wrote: > prefer to use gfx::Size instead of width/height params to avoid mixing up the > order (and reduce arities/increase readability) Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge.cc:177: env, j_mime.obj(), static_cast<jint>(width), static_cast<jint>(height))); On 2013/01/23 01:32:32, Ami Fischman wrote: > 80-cols Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge.cc:177: env, j_mime.obj(), static_cast<jint>(width), static_cast<jint>(height))); On 2013/01/23 01:32:32, Ami Fischman wrote: > Is it really necessary to static_cast an int to a jint?? Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge.cc:200: void MediaCodecBridge::Start() { On 2013/01/23 01:32:32, Ami Fischman wrote: > Why not make all configuration (audio/video) a single call and then always > Start() at the end of it? I guess it was because they have difference argument types. But, I think it can be done by passing MediaFormat as an argument. This is your intention, right? Added XXX for this refactoring. Please let me know if I misunderstood your suggestion. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge.cc:221: void MediaCodecBridge::Release() { On 2013/01/23 01:32:32, Ami Fischman wrote: > Hopefully you can drop this method in favor of the dtor. Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge.cc:312: if (j_input_buffers_.is_null()) return -1; On 2013/01/23 01:32:32, Ami Fischman wrote: > I don't see this in the javadoc. Removed. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge.cc:324: if (j_output_buffers_.is_null()) return -1; On 2013/01/23 01:32:32, Ami Fischman wrote: > again, javadoc doesn't support this. Removed. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge.cc:366: memcpy(data1, direct_buffer + size0, size1); On 2013/01/23 01:32:32, Ami Fischman wrote: > What guarantees that the planes are mashed together like this, without, e.g. > padding to a word or power-of-two boundary? This method will be removed and added when this is needed. Sorry for not cleaning up this in advance. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... File media/base/android/media_codec_bridge.h (right): https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge.h:13: //#include "base/memory/ref_counted.h" On 2013/01/23 01:32:32, Ami Fischman wrote: > remove prev 2 lines Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge.h:15: // This class serves as a bridge for native code to call java functions inside On 2013/01/23 01:32:32, Ami Fischman wrote: > Move down to immediately precede class declaration Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge.h:35: explicit MediaCodecBridge(const std::string& type); On 2013/01/23 01:32:32, Ami Fischman wrote: > s/type/mime_type/ Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge.h:39: void ConfigureAudio(const std::string& mime, int sample_rate, On 2013/01/23 01:32:32, Ami Fischman wrote: > s/mime/mime_type/ Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge.h:40: int channel_count, const uint8* csd0, int csd0_size, const uint8* csd1, On 2013/01/23 01:32:32, Ami Fischman wrote: > What's up with these csd0/1 params? > (why exactly 2 of them, why not use a vector<uint8> or a std::string to avoid > having to specify ptr/length separately, etc) Actually, they are optional params for codec specific data. I think It's better to make this clean and extend the function when it is needed. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge.h:44: const uint8* csd0, int csd0_size, const uint8* csd1, int csd1_size, On 2013/01/23 01:32:32, Ami Fischman wrote: > Ditto csd questions, and in fact I think you should just drop them entirely, as > the VDA should pump them in as part of the bitstream, anyway. Ditto. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge.h:48: void Flush(); On 2013/01/23 01:32:32, Ami Fischman wrote: > I think MediaCodec.flush() is something media:: code refers to as Reset(). > Please document what this does (drops all queued inputs on the floor, ready to > start decoding at a new seek point?) Actually, mediacodec's flush is somewhat different from omx's flush. Please see the comment I added. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge.h:49: void Stop(); On 2013/01/23 01:32:32, Ami Fischman wrote: > Stop() is irreversible in most media:: code. Please document. Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge.h:50: void Release(); On 2013/01/23 01:32:32, Ami Fischman wrote: > Again, please document what this does. Why would someone call this instead of > simply deleting the object? Can a Release()'d MediaCodecBridge object ever be > used again for any purpose? Agreed that this is not necessary in C++. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge.h:56: int DequeueInputBuffer(int64 timeout_us); On 2013/01/23 01:32:32, Ami Fischman wrote: > I question the value of providing a "waiting" API at this layer (in other words, > I think you want to omit this param and instead always pass 0 to the Java > layer). > > Does the 10us value you use in AVDA behave any differently from a value of 0? I made AVDA use 0us for timeout, but I think having timeout argument has value. For example, the timeout would be useful if someone needs to get the output index right after it is ready. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge.h:67: void GetFromOutputBuffer(int index, On 2013/01/23 01:32:32, Ami Fischman wrote: > Neither of the GetFromOutputBuffer methods is called; drop or annotate as > not-to-be-used for decode (where we use Surfaces)? Done. Surface is passed when we call configure(). https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge.h:80: base::android::ScopedJavaGlobalRef<jbyteArray> j_byte_array_; On 2013/01/23 01:32:32, Ami Fischman wrote: > Unused (both prev lines)? Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... File media/base/android/media_codec_bridge_unittest.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge_unittest.cc:93: class MediaCodecBridgeTest : public testing::Test { On 2013/01/23 01:32:32, Ami Fischman wrote: > This fixture adds a bunch of boilerplate but little value. > E.g. if you deleted it, the first test would simply turn into: > > TEST(MediaCodecBridgeTest, Initialize) { > MediaCodecBridge media_codec("audio/vorbis"); > } Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge_unittest.cc:112: Initialize("audio/vorbis"); On 2013/01/23 01:32:32, Ami Fischman wrote: > indent here and below should be +2 for test bodies (not +4 as you have it). Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge_unittest.cc:125: int input_buf_index; On 2013/01/23 01:32:32, Ami Fischman wrote: > Please declare variables as close to first use as possible (usually at first > assignment). Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge_unittest.cc:140: input_buf_index = media_codec_->DequeueInputBuffer(25000); On 2013/01/23 01:32:32, Ami Fischman wrote: > s/25000/0/ here and below. Changed to INFINITY. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge_unittest.cc:155: case MediaCodecBridge::INFO_TRY_AGAIN_LATER: On 2013/01/23 01:32:32, Ami Fischman wrote: > If you take my suggestion to remove the arbitrary timeout, you can drop this. Actually, this is possible output if we use 0. To fail on this case, I think we need to use -1(infinity). Please me know if I took your comment in a wrong way. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge_unittest.cc:158: case MediaCodecBridge::INFO_OUTPUT_FORMAT_CHANGED: On 2013/01/23 01:32:32, Ami Fischman wrote: > Shouldn't this FAIL() the test? This is a legitimate case also for audio. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge_unittest.cc:161: case MediaCodecBridge::INFO_OUTPUT_BUFFERS_CHANGED: On 2013/01/23 01:32:32, Ami Fischman wrote: > Since this is the only expected result, ASSERT it and drop the switch. Actually, if the index >= 0, that means a frame is decoded. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge_unittest.cc:166: if (!(flag & 4)) { On 2013/01/23 01:32:32, Ami Fischman wrote: > s/4/BUFFER_FLAG_END_OF_STREAM/ Done. https://chromiumcodereview.appspot.com/11973010/diff/6001/media/base/android/... media/base/android/media_codec_bridge_unittest.cc:169: } On 2013/01/23 01:32:32, Ami Fischman wrote: > This test would fail to fail if the decoder simply returned EOS immediately. I think the failure is legitimate because we passed two valid frames here. It should not simply return EOS immediately. Am I missing something? https://chromiumcodereview.appspot.com/11973010/diff/6001/media/media.gyp File media/media.gyp (right): https://chromiumcodereview.appspot.com/11973010/diff/6001/media/media.gyp#new... media/media.gyp:805: 'base/android/media_codec_bridge_unittest.cc', On 2013/01/23 01:32:32, Ami Fischman wrote: > I think this can go in the main sources list (platform-specific paths get > automatically omitted from other platform builds; see > http://src.chromium.org/viewvc/chrome/trunk/src/build/filename_rules.gypi?vie... > ) Thanks for the link! Done.
https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/6001/content/common/gpu/... content/common/gpu/media/android_video_decode_accelerator.cc:163: client_->NotifyEndOfBitstreamBuffer(bitstream_buffer.id()); On 2013/01/28 14:54:30, dwkang1 wrote: > On 2013/01/23 01:32:32, Ami Fischman wrote: > > This is wrong; you should only NotifyEndOfBitstreamBuffer when the contents of > > the buffer are known to have been decoded. > > In other words, it's not OK to call PictureReady with a bitstreambufferid of a > > bitstreambuffer for which you've already NEOBB'd. > > When I wrote this code, I referred OVDA code. In > OmxVideoDecodeAccelerator::EmptyBufferDoneTask(), which is a callback for input > feeding, it calls NEOBB(). Do you think OVDA's implementation is wrong also? See my comment on the next patchset. https://chromiumcodereview.appspot.com/11973010/diff/22001/base/android/jni_g... File base/android/jni_generator/jni_generator.py (right): https://chromiumcodereview.appspot.com/11973010/diff/22001/base/android/jni_g... base/android/jni_generator/jni_generator.py:175: return prefix + 'L' + param + ';' I suspect you meant to not include this file in this CL? https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:38: LOG(ERROR) << log; \ DLOG instead of LOG https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:78: && profile <= media::H264PROFILE_MAX) { && operator goes on previous line (here and elsewhere) https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:81: codec_ = media::MediaCodecBridge::UNKNOWN; unnecessary https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:113: if (state_ == NO_ERROR) { If this is false, then you don't want to enqueue more work, either. I think this just wants to be if (state_ == ERROR) return; instead. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:119: || !bitstream_buffer_ids_in_decoder_.empty()) { operator goes on previous line https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:126: io_task_is_running_ = false; IMO this would be more clear as io_task_is_posted_, which you'd set to true each time you Post'd DoIOTask, and which you cleared as the very first thing that DoIOTask did. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:135: media_codec_->DequeueInputBuffer(0); wrapping necessary? https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:137: return; DCHECK_EQ TRY_AGAIN_LATER? https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:169: client_->NotifyEndOfBitstreamBuffer(bitstream_buffer.id()); This is wrong - NEOBB() must only be called once its contents have been decoded (here, the buffer has only been handed to the decoder). In particular, clients are likely to keep a table of bitstreambufferid -> presentation-timestamp (and other metadata) and purge entries as bitstreambufferid's are notified done. But the entry is needed when a picture is delivered with a given bitstreambufferid, so NEOBB should only be called once you know no more pictures will be sent back with a given bitstreambufferid. Given the MediaCodec API, it may be that it is only correct to call NEOBB once input_buf_index is returned from DequeueInputBuffer() next. OVDA is not wrong - EmptyBufferDoneTask is a callback that the underlying OpenMAX library calls once it is done decoding from a buffer. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:192: return; braces are unnecessary (here and elsewhere in case tatements that don't introduce their own local variables; chromium/google3 style doesn't apply the usual if/then/else rule about if one clause has braces they all must to case statements). https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:210: RETURN_ON_FAILURE(size_.width() == width && size_.height() == height, size_ == gfx::Size(width, height) might be more readable https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:232: CHECK_EQ(bitstream_buffer_ids_in_decoder_.front(), bitstream_buffer_id); Presentation order does not have to match decode order, so I'm pretty sure this is wrong. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:233: bitstream_buffer_ids_in_decoder_.pop(); It's also wrong to assume that pictures & bitstreambuffers correspond 1:1 - a single bitstreambuffer can contain more than one picture, and a single picture can require multiple bitstreambuffers to decode. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:260: LOG(ERROR) << "Can't find a PuctureBuffer for " << picture_buffer_id; typo: PuctureBuffer https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:260: LOG(ERROR) << "Can't find a PuctureBuffer for " << picture_buffer_id; RETURN_ON_FAILURE https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:290: MessageLoop::current()->PostDelayedTask( Why not call directly? https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:305: output_picture_buffers_.insert(std::make_pair(buffers[i].id(), buffers[i])); output_picture_buffers_[buffers[i].id()] = buffers[i]; is slightly more readable. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:317: free_picture_ids_.push(picture_buffer_id); Doesn't this potentially require a DoIOTask? https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:331: media_codec_.reset(new media::MediaCodecBridge(codec_)); You don't need codec_ to re-configure a MediaCodec, since you can get the MediaFormat from GetOutputFormat. (but, see above, I hope you can get away from this re-creating business) https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:347: codec_, gfx::Size(1920, 1080), j_surface.obj()); Can you give more detail on what Exception MediaCodec threw when you used a 0x0 initial size? https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:364: client_->NotifyEndOfBitstreamBuffer(bitstream_buffer.id()); Please audit all uses of client_ to make sure they don't de-reference on a call-stack that could be rooted at the client itself (and Post instead of calling directly). Note that because of Destroy()'s contract, it's not enough to PostTask a Notify method on the client directly; you must pass through a trampoline method on *this* class that is guarded by a WeakPtr to avoid the case where a notification to the client is Posted, then Destroy() arrives and deletes *this*, and then the client gets an unexpected notification. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:372: ConfigureMediaCodec(); I still don't see why this is necessary; why not simply media_codec_->Start() at this point? https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:392: if (client_) { here and elsewhere, client_ cannot be non-NULL; drop the if's. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... File content/common/gpu/media/android_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.h:15: #include "base/threading/non_thread_safe.h" thread_checker.h, instead? https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... File content/common/gpu/media/gles2_external_texture_copier.h (right): https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.h:17: class Gles2ExternalTextureCopier { If you #include "ui/gl/gl_bindings.h" like r178150 made the rest of the VDAs do, you should be able to simply call glCopyTextureChromium from here and be fine, I think. https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... File media/base/android/media_codec_bridge.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.cc:53: *flags = static_cast<int>(env->GetIntField(buffer_info, flags_id)); are the static_cast<int>'s really necessary? https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.cc:74: if (!jni_initialized) { This is both racy and unreadable :) You have: static bool inited = false; if (!inited) { // Two threads can get here at the same time! Only function-static init is guaranteed atomicity! inited = ...; DCHECK(inited); } which can be written more readably as: static bool inited = ...; DCHECK(inited); https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.cc:77: && MediaCodecBufferInfo_RegisterNativesImpl(env) operator goes on previous line (here and elsewhere) https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.cc:86: typedef struct { This is C++; typedef struct { ... } Name; is more idiomatically typed as: struct Name { ... }; https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.cc:88: const char* mime_type_; de-indent 2 spaces https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.cc:99: #define NELEM(x) ((int) (sizeof(x) / sizeof((x)[0]))) Drop in favor of arraysize() https://code.google.com/p/chromium/codesearch#chrome/src/base/basictypes.h&q=... https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.cc:109: } l.86-109 can be replaced with the 9 lines: static const char* CodecToMimeType(const MediaCodecBridge::Codec codec) { switch (codec) { case MediaCodecBridge::AUDIO_MPEG: return "audio/mpeg"; case MediaCodecBridge::VIDEO_H264: return "video/avc"; case MediaCodecBridge::VIDEO_VP8: return "video/x-vnd.on2.vp8"; default: return NULL; } } Keep it simple :) https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.cc:244: return static_cast<int>(j_buffer); drop the cast? (here and elsewhere) https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.cc:256: int MediaCodecBridge::GetInputBuffers() { Why do you need to expose this as a public API of MCB? ISTM you can simply always call it immediately after start() returns, as an impl detail, no? https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.cc:288: memcpy(direct_buffer, data, size); I think you missed my comment about |size| being too big. You need to get the buffer's capacity first and ensure you're not going to be writing over random memory with this call. I suspect you'll need to make this method return bool instead of void and force callers to make sure they haven't overflowed. https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.cc:301: memcpy(data, direct_buffer + offset, size); Ditto you need some way to guarantee the requested size isn't going to be reading random memory. (but, again, I think you can drop this bit of API) https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... File media/base/android/media_codec_bridge.h (right): https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.h:25: UNKNOWN This isn't buying you anything except for extra cases to test for in random places. https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.h:48: // Configures MediaCodec with the given codec params. These do more than configure; they also Start(). Since there's a public Stop() method below, I think this deserves saying (and possibly renaming the methods to Start{Audio,Video}) https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.h:59: // words, there will be no outputs until new input is provided. How is this different from what media code calls Reset() (e.g. VDA::Reset())? https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.h:62: // Finishes the decode session. This is not informative. What happens to pending inputs & outputs? What can be called after this is called? https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.h:72: int64 presentation_time_us, int flags); Here & elsewhere, chromium code prefers base::TimeDelta to int64+unit suffix (like _us). https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.h:103: // Gets the data in the given output buffer. s/\.$/ (unnecessary when decoding video to a surface)./ https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... File media/base/android/media_codec_bridge_unittest.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge_unittest.cc:106: MediaCodecBridge::TIMEOUT_INFINITY); shouldn't this work with a 0 timeout, too? (which would be a stricter test) https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge_unittest.cc:111: int64 input_pts = 0; Using a base input_pts of something that's not -1/0/1 would make it easier to see if there was a bug. https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge_unittest.cc:139: continue; why not return? https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge_unittest.cc:141: case MediaCodecBridge::INFO_OUTPUT_FORMAT_CHANGED: This may be a legit API sequence for audio, but it should never trigger for this test data, so ISTM FAIL() is correct. https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge_unittest.cc:145: media_codec.GetOutputBuffers(); Again, since we don't expect the test data to trigger this, FAIL(). https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge_unittest.cc:150: EXPECT_EQ(++input_pts, output_pts); The input queues start with a pts of 1; how come the output pts's start at 0?
https://chromiumcodereview.appspot.com/11973010/diff/22001/base/android/jni_g... File base/android/jni_generator/jni_generator.py (right): https://chromiumcodereview.appspot.com/11973010/diff/22001/base/android/jni_g... base/android/jni_generator/jni_generator.py:175: return prefix + 'L' + param + ';' On 2013/01/28 19:49:45, Ami Fischman wrote: > I suspect you meant to not include this file in this CL? I just added this to check this fix works well with MediaCodec. Let me remove this with rebase once the separate change (https://chromiumcodereview.appspot.com/12094008/) is submitted. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:169: client_->NotifyEndOfBitstreamBuffer(bitstream_buffer.id()); On 2013/01/28 19:49:45, Ami Fischman wrote: > This is wrong - NEOBB() must only be called once its contents have been decoded > (here, the buffer has only been handed to the decoder). > In particular, clients are likely to keep a table of bitstreambufferid -> > presentation-timestamp (and other metadata) and purge entries as > bitstreambufferid's are notified done. But the entry is needed when a picture > is delivered with a given bitstreambufferid, so NEOBB should only be called once > you know no more pictures will be sent back with a given bitstreambufferid. > > Given the MediaCodec API, it may be that it is only correct to call NEOBB once > input_buf_index is returned from DequeueInputBuffer() next. > > OVDA is not wrong - EmptyBufferDoneTask is a callback that the underlying > OpenMAX library calls once it is done decoding from a buffer. Got it. Seems that I misunderstood the meaning of EmptyBufferDone. Will fix in the next patch set. Thanks for the explanation! https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:372: ConfigureMediaCodec(); On 2013/01/28 19:49:45, Ami Fischman wrote: > I still don't see why this is necessary; why not simply media_codec_->Start() at > this point? When I tested "playback complete" and "replay" case, MediaCodec became unusable once it meets EOS. Seems that this is another behavior which is not covered in the api doc. Let me verify this, with Android team, and add some more comments regarding this. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... File content/common/gpu/media/gles2_external_texture_copier.h (right): https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.h:17: class Gles2ExternalTextureCopier { On 2013/01/28 19:49:45, Ami Fischman wrote: > If you #include "ui/gl/gl_bindings.h" > like r178150 made the rest of the VDAs do, you should be able to simply call > glCopyTextureChromium from here and be fine, I think. Will try in the next patch. Thanks. But, transfrom matrix from SurfaceTexture might be a blocker. Let me take a more look and get back to you. https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... File media/base/android/media_codec_bridge.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.cc:288: memcpy(direct_buffer, data, size); On 2013/01/28 19:49:45, Ami Fischman wrote: > I think you missed my comment about |size| being too big. > You need to get the buffer's capacity first and ensure you're not going to be > writing over random memory with this call. > I suspect you'll need to make this method return bool instead of void and force > callers to make sure they haven't overflowed. Sorry for the missing your previous comment. Will fix in this next patch set!
https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:38: LOG(ERROR) << log; \ On 2013/01/28 19:49:45, Ami Fischman wrote: > DLOG instead of LOG Done. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:78: && profile <= media::H264PROFILE_MAX) { On 2013/01/28 19:49:45, Ami Fischman wrote: > && operator goes on previous line (here and elsewhere) Done. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:81: codec_ = media::MediaCodecBridge::UNKNOWN; On 2013/01/28 19:49:45, Ami Fischman wrote: > unnecessary Done. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:113: if (state_ == NO_ERROR) { On 2013/01/28 19:49:45, Ami Fischman wrote: > If this is false, then you don't want to enqueue more work, either. > I think this just wants to be > if (state_ == ERROR) > return; > instead. Done. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:119: || !bitstream_buffer_ids_in_decoder_.empty()) { On 2013/01/28 19:49:45, Ami Fischman wrote: > operator goes on previous line Done. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:126: io_task_is_running_ = false; On 2013/01/28 19:49:45, Ami Fischman wrote: > IMO this would be more clear as io_task_is_posted_, which you'd set to true each > time you Post'd DoIOTask, and which you cleared as the very first thing that > DoIOTask did. Good idea. Thanks, https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:135: media_codec_->DequeueInputBuffer(0); On 2013/01/28 19:49:45, Ami Fischman wrote: > wrapping necessary? Done. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:137: return; On 2013/01/28 19:49:45, Ami Fischman wrote: > DCHECK_EQ TRY_AGAIN_LATER? Done. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:169: client_->NotifyEndOfBitstreamBuffer(bitstream_buffer.id()); On 2013/01/29 03:58:48, dwkang1 wrote: > On 2013/01/28 19:49:45, Ami Fischman wrote: > > This is wrong - NEOBB() must only be called once its contents have been > decoded > > (here, the buffer has only been handed to the decoder). > > In particular, clients are likely to keep a table of bitstreambufferid -> > > presentation-timestamp (and other metadata) and purge entries as > > bitstreambufferid's are notified done. But the entry is needed when a picture > > is delivered with a given bitstreambufferid, so NEOBB should only be called > once > > you know no more pictures will be sent back with a given bitstreambufferid. > > > > Given the MediaCodec API, it may be that it is only correct to call NEOBB once > > input_buf_index is returned from DequeueInputBuffer() next. > > > > OVDA is not wrong - EmptyBufferDoneTask is a callback that the underlying > > OpenMAX library calls once it is done decoding from a buffer. > > Got it. Seems that I misunderstood the meaning of EmptyBufferDone. Will fix in > the next patch set. Thanks for the explanation! I checked GpuVideoDecoder implementation and have a question on it. Is the table of bitstreambufferid -> presentation-timestamp |input_buffer_data_| in GpuVideoDecoder? IIRC, I don't see any relationship between |input_buffer_data_| and NEOBB. It was just keeping last 128 entries to provide the mapping and be reset on Reset(). And, GpuVideoDecoder::NotifyEndOfBitstreamBuffer() did nothing with |input_buffer_data_|. Actually, What happened in GpuVideoDecoder::NotifyEndOfBitstreamBuffer() was remove the matching id in |bitstream_buffers_in_decoder_| and # of |bitstream_buffers_in_decoder_| was limited by 4(kMaxInFlightDecodes). When you say "once its contents have been decoded", does that mean "once we get decoded output from the contents"? If so, it sounds to me that the decoder should be able to start outputting decoded frames after getting 4 bitstream buffers. However, in my test streams, decoder started outputting frames after feeding 8-9 bitstream buffers. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:192: return; On 2013/01/28 19:49:45, Ami Fischman wrote: > braces are unnecessary (here and elsewhere in case tatements that don't > introduce their own local variables; chromium/google3 style doesn't apply the > usual if/then/else rule about if one clause has braces they all must to case > statements). Done. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:210: RETURN_ON_FAILURE(size_.width() == width && size_.height() == height, On 2013/01/28 19:49:45, Ami Fischman wrote: > size_ == gfx::Size(width, height) > might be more readable Done. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:232: CHECK_EQ(bitstream_buffer_ids_in_decoder_.front(), bitstream_buffer_id); On 2013/01/28 19:49:45, Ami Fischman wrote: > Presentation order does not have to match decode order, so I'm pretty sure this > is wrong. Agreed. Haste made waste. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:233: bitstream_buffer_ids_in_decoder_.pop(); On 2013/01/28 19:49:45, Ami Fischman wrote: > It's also wrong to assume that pictures & bitstreambuffers correspond 1:1 - a > single bitstreambuffer can contain more than one picture, and a single picture > can require multiple bitstreambuffers to decode. You are correct. removed. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:260: LOG(ERROR) << "Can't find a PuctureBuffer for " << picture_buffer_id; On 2013/01/28 19:49:45, Ami Fischman wrote: > typo: PuctureBuffer Done. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:260: LOG(ERROR) << "Can't find a PuctureBuffer for " << picture_buffer_id; On 2013/01/28 19:49:45, Ami Fischman wrote: > RETURN_ON_FAILURE Done. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:290: MessageLoop::current()->PostDelayedTask( On 2013/01/28 19:49:45, Ami Fischman wrote: > Why not call directly? Done. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:305: output_picture_buffers_.insert(std::make_pair(buffers[i].id(), buffers[i])); On 2013/01/28 19:49:45, Ami Fischman wrote: > output_picture_buffers_[buffers[i].id()] = buffers[i]; > is slightly more readable. It lead a compile error because PictureBuffer does not have default c-tor. http://stackoverflow.com/questions/8279934/error-no-matching-function-for-cal... https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:317: free_picture_ids_.push(picture_buffer_id); On 2013/01/28 19:49:45, Ami Fischman wrote: > Doesn't this potentially require a DoIOTask? Done. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:331: media_codec_.reset(new media::MediaCodecBridge(codec_)); On 2013/01/28 19:49:45, Ami Fischman wrote: > You don't need codec_ to re-configure a MediaCodec, since you can get the > MediaFormat from GetOutputFormat. > (but, see above, I hope you can get away from this re-creating business) Unfortunately, I was not able to drop the recreation because of EOS->flush()->dequeueInputBuffer() issue. And, MediaCodec's getOutputFormat() returns the format of output, mime type : "video/raw". https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:347: codec_, gfx::Size(1920, 1080), j_surface.obj()); On 2013/01/28 19:49:45, Ami Fischman wrote: > Can you give more detail on what Exception MediaCodec threw when you used a 0x0 > initial size? Here is the logcat output when I pass 0x0 on Nexus10: W/GraphicBufferAllocator( 124): alloc(1, 1, 263, 00002100, ...) failed -22 (Invalid argument) E/SurfaceFlinger( 124): GraphicBufferAlloc::createGraphicBuffer(w=1, h=1) failed (Invalid argument), handle=0x0 E/BufferQueue(28661): [unnamed-28661-0] dequeueBuffer: SurfaceComposer::createGraphicBuffer failed E/ACodec (28661): dequeueBuffer failed: Invalid argument (22) E/ACodec (28661): Failed to allocate buffers after transitioning to IDLE state (error 0xffffffea) E/MediaCodec(28661): Codec reported an error. (omx error 0x80001001, internalError -22) W/System.err(28661): java.lang.IllegalStateException W/System.err(28661): at android.media.MediaCodec.start(Native Method) W/System.err(28661): at dalvik.system.NativeStart.run(Native Method) FWIW, passing 1x1 also failed with similar output. A arbitrary small resolution(320x240) worked, even though I tested with full-hd stream, but I don't think this is the way to go. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:364: client_->NotifyEndOfBitstreamBuffer(bitstream_buffer.id()); On 2013/01/28 19:49:45, Ami Fischman wrote: > Please audit all uses of client_ to make sure they don't de-reference on a > call-stack that could be rooted at the client itself (and Post instead of > calling directly). > Note that because of Destroy()'s contract, it's not enough to PostTask a Notify > method on the client directly; you must pass through a trampoline method on > *this* class that is guarded by a WeakPtr to avoid the case where a notification > to the client is Posted, then Destroy() arrives and deletes *this*, and then the > client gets an unexpected notification. Done. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:372: ConfigureMediaCodec(); On 2013/01/29 03:58:48, dwkang1 wrote: > On 2013/01/28 19:49:45, Ami Fischman wrote: > > I still don't see why this is necessary; why not simply media_codec_->Start() > at > > this point? > > When I tested "playback complete" and "replay" case, MediaCodec became unusable > once it meets EOS. Seems that this is another behavior which is not covered in > the api doc. Let me verify this, with Android team, and add some more comments > regarding this. As you may know, we had a discussion with Android team, and it turned out a bug on vendor's implementation. Considering that it happens many devices, I think we need to have some workaround code. I changed to the code to do the recreation only when we met EOS. PTAL. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:392: if (client_) { On 2013/01/28 19:49:45, Ami Fischman wrote: > here and elsewhere, client_ cannot be non-NULL; drop the if's. Done. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... File content/common/gpu/media/android_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.h:15: #include "base/threading/non_thread_safe.h" On 2013/01/28 19:49:45, Ami Fischman wrote: > thread_checker.h, instead? Done. https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... File content/common/gpu/media/gles2_external_texture_copier.h (right): https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.h:17: class Gles2ExternalTextureCopier { On 2013/01/29 03:58:48, dwkang1 wrote: > On 2013/01/28 19:49:45, Ami Fischman wrote: > > If you #include "ui/gl/gl_bindings.h" > > like r178150 made the rest of the VDAs do, you should be able to simply call > > glCopyTextureChromium from here and be fine, I think. > > Will try in the next patch. Thanks. But, transfrom matrix from SurfaceTexture > might be a blocker. Let me take a more look and get back to you. When I include "ui/gl/gl_bindings.h" and use glCopyTextureChromium, I met an error which says it is not declared. Will take more look and please let me know if you know anything about this. Thanks, https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... File media/base/android/media_codec_bridge.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.cc:53: *flags = static_cast<int>(env->GetIntField(buffer_info, flags_id)); On 2013/01/28 19:49:45, Ami Fischman wrote: > are the static_cast<int>'s really necessary? Removed. https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.cc:53: *flags = static_cast<int>(env->GetIntField(buffer_info, flags_id)); On 2013/01/28 19:49:45, Ami Fischman wrote: > are the static_cast<int>'s really necessary? Removed. https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.cc:74: if (!jni_initialized) { On 2013/01/28 19:49:45, Ami Fischman wrote: > This is both racy and unreadable :) > > You have: > > static bool inited = false; > if (!inited) { // Two threads can get here at the same time! Only > function-static init is guaranteed atomicity! > inited = ...; > DCHECK(inited); > } > > > which can be written more readably as: > static bool inited = ...; > DCHECK(inited); Thanks for the advise. btw, do you have any reference about atomicity of the second approach? https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.cc:77: && MediaCodecBufferInfo_RegisterNativesImpl(env) On 2013/01/28 19:49:45, Ami Fischman wrote: > operator goes on previous line (here and elsewhere) Done. https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.cc:86: typedef struct { On 2013/01/28 19:49:45, Ami Fischman wrote: > This is C++; > typedef struct { ... } Name; > is more idiomatically typed as: > struct Name { ... }; Done. https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.cc:88: const char* mime_type_; On 2013/01/28 19:49:45, Ami Fischman wrote: > de-indent 2 spaces Done. https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.cc:99: #define NELEM(x) ((int) (sizeof(x) / sizeof((x)[0]))) On 2013/01/28 19:49:45, Ami Fischman wrote: > Drop in favor of arraysize() > https://code.google.com/p/chromium/codesearch#chrome/src/base/basictypes.h&q=... Thanks for the link. https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.cc:109: } On 2013/01/28 19:49:45, Ami Fischman wrote: > l.86-109 can be replaced with the 9 lines: > static const char* CodecToMimeType(const MediaCodecBridge::Codec codec) { > switch (codec) { > case MediaCodecBridge::AUDIO_MPEG: return "audio/mpeg"; > case MediaCodecBridge::VIDEO_H264: return "video/avc"; > case MediaCodecBridge::VIDEO_VP8: return "video/x-vnd.on2.vp8"; > default: > return NULL; > } > } > > Keep it simple :) Done. https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.cc:244: return static_cast<int>(j_buffer); On 2013/01/28 19:49:45, Ami Fischman wrote: > drop the cast? (here and elsewhere) Done. https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.cc:256: int MediaCodecBridge::GetInputBuffers() { On 2013/01/28 19:49:45, Ami Fischman wrote: > Why do you need to expose this as a public API of MCB? ISTM you can simply > always call it immediately after start() returns, as an impl detail, no? According to the api doc we need to call getOutputBuffers() when INFO_OUTPUT_BUFFERS_CHANGED is returned. So GOB() should be public. In GIB() case, we can make it private and call it in Start(), but it doesn't look consistent to me. WDYT? https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.cc:288: memcpy(direct_buffer, data, size); On 2013/01/29 03:58:48, dwkang1 wrote: > On 2013/01/28 19:49:45, Ami Fischman wrote: > > I think you missed my comment about |size| being too big. > > You need to get the buffer's capacity first and ensure you're not going to be > > writing over random memory with this call. > > I suspect you'll need to make this method return bool instead of void and > force > > callers to make sure they haven't overflowed. > > Sorry for the missing your previous comment. Will fix in this next patch set! Changed to return # of bytes written. https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.cc:301: memcpy(data, direct_buffer + offset, size); On 2013/01/28 19:49:45, Ami Fischman wrote: > Ditto you need some way to guarantee the requested size isn't going to be > reading random memory. > (but, again, I think you can drop this bit of API) Dropped. :) https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... File media/base/android/media_codec_bridge.h (right): https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.h:25: UNKNOWN On 2013/01/28 19:49:45, Ami Fischman wrote: > This isn't buying you anything except for extra cases to test for in random > places. Done. https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.h:48: // Configures MediaCodec with the given codec params. On 2013/01/28 19:49:45, Ami Fischman wrote: > These do more than configure; they also Start(). > Since there's a public Stop() method below, I think this deserves saying (and > possibly renaming the methods to Start{Audio,Video}) Done. https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.h:59: // words, there will be no outputs until new input is provided. On 2013/01/28 19:49:45, Ami Fischman wrote: > How is this different from what media code calls Reset() (e.g. VDA::Reset())? It should be the same. But, there is a difference between MediaCodec.flush() and OpenMaxIL's flush. In OpenMax, when we call flush against ports, they return the buffers passed to the component to the IL client. https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.h:62: // Finishes the decode session. On 2013/01/28 19:49:45, Ami Fischman wrote: > This is not informative. > What happens to pending inputs & outputs? > What can be called after this is called? More description is added, but when I tested stop() to check the pending inputs & outputs, start() after stop() threw IllegalStateException. So, I could not check the status after stop(). Will check this behavior with Android team. https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.h:72: int64 presentation_time_us, int flags); On 2013/01/28 19:49:45, Ami Fischman wrote: > Here & elsewhere, chromium code prefers base::TimeDelta to int64+unit suffix > (like _us). Done. https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge.h:103: // Gets the data in the given output buffer. On 2013/01/28 19:49:45, Ami Fischman wrote: > s/\.$/ (unnecessary when decoding video to a surface)./ Done. https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... File media/base/android/media_codec_bridge_unittest.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge_unittest.cc:106: MediaCodecBridge::TIMEOUT_INFINITY); On 2013/01/28 19:49:45, Ami Fischman wrote: > shouldn't this work with a 0 timeout, too? > (which would be a stricter test) I found that just using 0 timeout makes this test flaky. MediaCodec may return INFO_TRY_AGAIN_LATER. https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge_unittest.cc:111: int64 input_pts = 0; On 2013/01/28 19:49:45, Ami Fischman wrote: > Using a base input_pts of something that's not -1/0/1 would make it easier to > see if there was a bug. Done. https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge_unittest.cc:139: continue; On 2013/01/28 19:49:45, Ami Fischman wrote: > why not return? Done. https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge_unittest.cc:141: case MediaCodecBridge::INFO_OUTPUT_FORMAT_CHANGED: On 2013/01/28 19:49:45, Ami Fischman wrote: > This may be a legit API sequence for audio, but it should never trigger for this > test data, so ISTM FAIL() is correct. Actually, this is returned also in audio case. We can get meta data like sampling rate and channel. (But, MediaCodecBridge class does not have api for getting the meta yet.) https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge_unittest.cc:145: media_codec.GetOutputBuffers(); On 2013/01/28 19:49:45, Ami Fischman wrote: > Again, since we don't expect the test data to trigger this, FAIL(). In the api doc, there is no mention that it will not happen in audio case. http://developer.android.com/reference/android/media/MediaCodec.html#INFO_OUT... https://chromiumcodereview.appspot.com/11973010/diff/22001/media/base/android... media/base/android/media_codec_bridge_unittest.cc:150: EXPECT_EQ(++input_pts, output_pts); On 2013/01/28 19:49:45, Ami Fischman wrote: > The input queues start with a pts of 1; how come the output pts's start at 0? It starts from 1. Please note that I used *++*input_pts and it is reset to 0 at L127.
https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:76: DCHECK(media_codec_ == NULL); !media_codec_ https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:136: if (pending_bitstream_buffers_.empty()) { no { https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:148: if (bitstream_buffer.id() == -1) { ditto https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:190: if (picturebuffers_requested_ && output_picture_buffers_.empty()) { ditto https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:246: if (bitstream_buffer_id != -1) { ditto here and below https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:299: if (!io_task_is_posted_) { ditto https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:325: if (!io_task_is_posted_) { ditto https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:357: codec_, gfx::Size(1920, 1080), j_surface.obj()); why 1080p? most android devices are 720p at most https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... File content/common/gpu/media/gles2_external_texture_copier.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.cc:38: enum { kVerticeStride = 5 * sizeof(float) }; why enum? https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.cc:89: if (!vertexShader) { no { https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.cc:94: if (!pixelShader) { ditto https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.cc:227: return false; ditto https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... File media/base/android/media_codec_bridge.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... media/base/android/media_codec_bridge.cc:245: env, j_media_codec_.obj(), index, static_cast<jboolean>(render)); do we need static_cast for this? I think boolean and jboolean are of the same type underneath. https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... File media/base/android/media_codec_bridge_unittest.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... media/base/android/media_codec_bridge_unittest.cc:93: enum { kPresentationTimeBase = 100 }; should this be a static const? https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... media/base/android/media_codec_bridge_unittest.cc:154: if (!(flag & MediaCodecBridge::BUFFER_FLAG_END_OF_STREAM)) { no {
Thanks for the review and I have one quick question. https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:136: if (pending_bitstream_buffers_.empty()) { On 2013/02/05 07:43:13, qinmin wrote: > no { afaik, it is okay to have { if it is consistent in a file in under google3. does chrome have different rule?
https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:76: DCHECK(media_codec_ == NULL); On 2013/02/05 07:43:13, qinmin wrote: > !media_codec_ Done. https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:148: if (bitstream_buffer.id() == -1) { On 2013/02/05 07:43:13, qinmin wrote: > ditto Done. https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:190: if (picturebuffers_requested_ && output_picture_buffers_.empty()) { On 2013/02/05 07:43:13, qinmin wrote: > ditto Done. https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:246: if (bitstream_buffer_id != -1) { On 2013/02/05 07:43:13, qinmin wrote: > ditto here and below Done. https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:299: if (!io_task_is_posted_) { On 2013/02/05 07:43:13, qinmin wrote: > ditto Done. https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:325: if (!io_task_is_posted_) { On 2013/02/05 07:43:13, qinmin wrote: > ditto Done. https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:357: codec_, gfx::Size(1920, 1080), j_surface.obj()); On 2013/02/05 07:43:13, qinmin wrote: > why 1080p? most android devices are 720p at most Changed to 720p. https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... File content/common/gpu/media/gles2_external_texture_copier.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.cc:38: enum { kVerticeStride = 5 * sizeof(float) }; On 2013/02/05 07:43:13, qinmin wrote: > why enum? Done. https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.cc:89: if (!vertexShader) { On 2013/02/05 07:43:13, qinmin wrote: > no { Done. https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.cc:94: if (!pixelShader) { On 2013/02/05 07:43:13, qinmin wrote: > ditto Done. https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.cc:227: return false; On 2013/02/05 07:43:13, qinmin wrote: > ditto Done. https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... File media/base/android/media_codec_bridge.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... media/base/android/media_codec_bridge.cc:245: env, j_media_codec_.obj(), index, static_cast<jboolean>(render)); On 2013/02/05 07:43:13, qinmin wrote: > do we need static_cast for this? I think boolean and jboolean are of the same > type underneath. Done. https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... File media/base/android/media_codec_bridge_unittest.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... media/base/android/media_codec_bridge_unittest.cc:93: enum { kPresentationTimeBase = 100 }; On 2013/02/05 07:43:13, qinmin wrote: > should this be a static const? Done. https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... media/base/android/media_codec_bridge_unittest.cc:154: if (!(flag & MediaCodecBridge::BUFFER_FLAG_END_OF_STREAM)) { On 2013/02/05 07:43:13, qinmin wrote: > no { Done.
https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:31: #define DCHECK CHECK Oh my! :) You might enjoy https://code.google.com/searchframe#OAMlx_jo-ck/src/base/base_switches.cc&exa... https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:139: int input_buf_index = media_codec_->DequeueInputBuffer(base::TimeDelta()); kNoWait or somesuch? Otherwise reading this callsite it's not obvious what the TimeDelta parameter is about. https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:182: if (bitstream_buffer.id() != -1) { On 2013/02/04 14:08:26, dwkang1 wrote: > On 2013/01/29 03:58:48, dwkang1 wrote: > > On 2013/01/28 19:49:45, Ami Fischman wrote: > > > This is wrong - NEOBB() must only be called once its contents have been > > decoded > > > (here, the buffer has only been handed to the decoder). > > > In particular, clients are likely to keep a table of bitstreambufferid -> > > > presentation-timestamp (and other metadata) and purge entries as > > > bitstreambufferid's are notified done. But the entry is needed when a > picture > > > is delivered with a given bitstreambufferid, so NEOBB should only be called > > once > > > you know no more pictures will be sent back with a given bitstreambufferid. > > > > > > Given the MediaCodec API, it may be that it is only correct to call NEOBB > once > > > input_buf_index is returned from DequeueInputBuffer() next. > > > > > > OVDA is not wrong - EmptyBufferDoneTask is a callback that the underlying > > > OpenMAX library calls once it is done decoding from a buffer. > > > > Got it. Seems that I misunderstood the meaning of EmptyBufferDone. Will fix in > > the next patch set. Thanks for the explanation! > > I checked GpuVideoDecoder implementation and have a question on it. Is the table > of bitstreambufferid -> > presentation-timestamp |input_buffer_data_| in GpuVideoDecoder? Yes. > IIRC, I don't see any relationship between |input_buffer_data_| and NEOBB. It > was just keeping last 128 entries to provide the mapping and be reset on > Reset(). And, GpuVideoDecoder::NotifyEndOfBitstreamBuffer() did nothing with > |input_buffer_data_|. Correct. But GVD is not the only client of VDA - PPAPI is another, and you can't know what plugin authors are doing with this, so you should obey the contract of the API. > When you say "once its contents have been decoded", does that mean "once we get > decoded output from the contents"? No. I mean "once it is guaranteed that no more decoded output will be returned from the contents". Maybe it's not possible to implement this given MediaCodec's API? > should be able to start outputting decoded frames after getting 4 bitstream > buffers. However, in my test streams, decoder started outputting frames after > feeding 8-9 bitstream buffers. Usually the first few buffers contain SPS/PPS and other stream initialization data. https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:184: &AndroidVideoDecodeAccelerator::NotifyEndOfBitstreamBuffer, This is still wrong - https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:225: RETURN_ON_FAILURE(size_ == gfx::Size(width, height), So, what if this == is true? Ignore changes in color format? Can something else have changed other than colorformat/width/height? https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:240: &AndroidVideoDecodeAccelerator::NotifyFlushDone, base::AsWeakPtr(this))); 80-cols https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:263: RETURN_ON_FAILURE(make_context_current_.Run(), Do this before the pop above? https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:304: void AndroidVideoDecodeAccelerator::AssignPictureBuffers( Does this need to DoIOTask as well? (not sure, it's getting late ;)) https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:357: codec_, gfx::Size(1920, 1080), j_surface.obj()); to ensure we don't hide bugs, can we miminize this (16x16, maybe?)? https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:384: // devices. To avoid the case, we recreate a new one. This sort of comment should really have a pointer to a filed bug (probably on the android bug tracker) https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:399: delete this; Do you want to stop the codec first? https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... File content/common/gpu/media/android_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.h:20: class MessageLoop; unused https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.h:111: // The current state of this class. For now, this is used for setting error s/for/only for/ https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.h:115: // This map maintains the picture buffers passed the client for decoding. s/passed/passed to/ https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... File content/common/gpu/media/gles2_external_texture_copier.h (right): https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.h:14: // A utility class which copies the given external texture What happened to using the proper gl headers? https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... File media/base/android/media_codec_bridge.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... media/base/android/media_codec_bridge.cc:72: static bool jni_initialized = Thanks for asking about thread-safety of this approach. Turns out it's not safe for chromium! See https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/p6h3HC8Wro4... for relevant info. So, apparently the thing to do is to replace this with a LazyInstance. :( https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... media/base/android/media_codec_bridge.cc:74: && MediaCodecBufferInfo_RegisterNativesImpl(env) Operator (&&) belongs on previous line, here and elsewhere. https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... media/base/android/media_codec_bridge.cc:248: int MediaCodecBridge::GetInputBuffers() { I think given the two unattractive facts: 1) An asymmetric API (GOB exists but GIB doesn't) 2) A method is exposed that is called exactly one exactly after another method and so could have been inlined into it I'd rather my API have #1 than #2. https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... File media/base/android/media_codec_bridge.h (right): https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... media/base/android/media_codec_bridge.h:31: BUFFER_FLAG_FLAG_SYNC_FRAME = 1, Neither of the latter two flags is used. Are you sure they're worth including at this point? https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... media/base/android/media_codec_bridge.h:50: const Codec codec, const gfx::Size& size, jobject surface); Actually, instead of feeding in a dummy size, how about dropping the param here and using the dummy value (16x16 or somesuch only in bridge.cc)? https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... media/base/android/media_codec_bridge.h:56: void Flush(); My point was that this would be better named Reset() to match other media:: code. https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... media/base/android/media_codec_bridge.h:85: base::TimeDelta& presentation_time, int* flags); style: no non-const refs in chromium. s/base::TimeDelta&/base::TimeDelta*/ https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... media/base/android/media_codec_bridge.h:93: size_t PutToInputBuffer(int index, const uint8* data, int size); Why does GIB() still make sense as an API? Wouldn't it make sense to combine that and this into a single method? (and, in fact, since the only flag you use is EOS, could drop the magical value of 0-size buffers and replace with a bool EnqueueEOS(); https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... File media/base/android/media_codec_bridge_unittest.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... media/base/android/media_codec_bridge_unittest.cc:151: return; Did you mean to FAIL() here?
https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:31: #define DCHECK CHECK On 2013/02/05 20:24:33, Ami Fischman wrote: > Oh my! :) > You might enjoy > https://code.google.com/searchframe#OAMlx_jo-ck/src/base/base_switches.cc&exa... haha. It was part of XXX at L27. :) Thanks for letting me know the flag. Looks very useful! https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:139: int input_buf_index = media_codec_->DequeueInputBuffer(base::TimeDelta()); On 2013/02/05 20:24:33, Ami Fischman wrote: > kNoWait or somesuch? Otherwise reading this callsite it's not obvious what the > TimeDelta parameter is about. Good idea. https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:184: &AndroidVideoDecodeAccelerator::NotifyEndOfBitstreamBuffer, On 2013/02/05 20:24:33, Ami Fischman wrote: > This is still wrong - > https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... Actually, I implemented the scheme you mentioned (keeping bitstream ids in input buffers and calling NEOBB when the input buffer id is returned again). However, it didn't work because the # of input buffers was more than 4 and kMaxInFlightDecodes in gpu_video_decoder was also 4. gpu_video_decoder didn't provider bitstream buffer more than four. :( BTW, can I ask the background of having "once it is guaranteed that no more decoded output will be returned from the contents" semantics? It looks fine to me because the bitstream is copied when it is passed to the input buffer. Was there any specific use case? https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:225: RETURN_ON_FAILURE(size_ == gfx::Size(width, height), On 2013/02/05 20:24:33, Ami Fischman wrote: > So, what if this == is true? Ignore changes in color format? Can something > else have changed other than colorformat/width/height? In the decoder case, only width and height matter. So, I removed color format. http://developer.android.com/reference/android/media/MediaFormat.html https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:240: &AndroidVideoDecodeAccelerator::NotifyFlushDone, base::AsWeakPtr(this))); On 2013/02/05 20:24:33, Ami Fischman wrote: > 80-cols Done. https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:263: RETURN_ON_FAILURE(make_context_current_.Run(), On 2013/02/05 20:24:33, Ami Fischman wrote: > Do this before the pop above? Done. https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:304: void AndroidVideoDecodeAccelerator::AssignPictureBuffers( On 2013/02/05 20:24:33, Ami Fischman wrote: > Does this need to DoIOTask as well? (not sure, it's getting late ;)) Yes, it does. https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:357: codec_, gfx::Size(1920, 1080), j_surface.obj()); On 2013/02/05 20:24:33, Ami Fischman wrote: > to ensure we don't hide bugs, can we miminize this (16x16, maybe?)? I tried from 16x16 ~ 256x256 because 16 ~ 128 showed show some broken area in the texture. 256 was fine, but now I think using predefined value is not a good idea. It seems to me that getting the container indicated resolution via vda could be a resolution. Do you see any problem with this approach? https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:384: // devices. To avoid the case, we recreate a new one. On 2013/02/05 20:24:33, Ami Fischman wrote: > This sort of comment should really have a pointer to a filed bug (probably on > the android bug tracker) bug id is added. https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:399: delete this; On 2013/02/05 20:24:33, Ami Fischman wrote: > Do you want to stop the codec first? Done. https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... File content/common/gpu/media/android_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.h:20: class MessageLoop; On 2013/02/05 20:24:33, Ami Fischman wrote: > unused Done. https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.h:111: // The current state of this class. For now, this is used for setting error On 2013/02/05 20:24:33, Ami Fischman wrote: > s/for/only for/ Done. https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.h:115: // This map maintains the picture buffers passed the client for decoding. On 2013/02/05 20:24:33, Ami Fischman wrote: > s/passed/passed to/ Done. https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... File content/common/gpu/media/gles2_external_texture_copier.h (right): https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.h:14: // A utility class which copies the given external texture On 2013/02/05 20:24:33, Ami Fischman wrote: > What happened to using the proper gl headers? I was not able to handle this today. Let me mark this with XXX and handle this in the next patch set. https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... File media/base/android/media_codec_bridge.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... media/base/android/media_codec_bridge.cc:72: static bool jni_initialized = On 2013/02/05 20:24:33, Ami Fischman wrote: > Thanks for asking about thread-safety of this approach. Turns out it's not safe > for chromium! > See > https://groups.google.com/a/chromium.org/forum/#%21msg/chromium-dev/p6h3HC8Wr... > for relevant info. > > So, apparently the thing to do is to replace this with a LazyInstance. > :( Changed by using LazyInstance. https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... media/base/android/media_codec_bridge.cc:74: && MediaCodecBufferInfo_RegisterNativesImpl(env) On 2013/02/05 20:24:33, Ami Fischman wrote: > Operator (&&) belongs on previous line, here and elsewhere. Done. https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... media/base/android/media_codec_bridge.cc:248: int MediaCodecBridge::GetInputBuffers() { On 2013/02/05 20:24:33, Ami Fischman wrote: > I think given the two unattractive facts: > 1) An asymmetric API (GOB exists but GIB doesn't) > 2) A method is exposed that is called exactly one exactly after another method > and so could have been inlined into it > I'd rather my API have #1 than #2. Okay. I'll include this to Start(). https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... File media/base/android/media_codec_bridge.h (right): https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... media/base/android/media_codec_bridge.h:31: BUFFER_FLAG_FLAG_SYNC_FRAME = 1, On 2013/02/05 20:24:33, Ami Fischman wrote: > Neither of the latter two flags is used. Are you sure they're worth including > at this point? Removed. https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... media/base/android/media_codec_bridge.h:56: void Flush(); On 2013/02/05 20:24:33, Ami Fischman wrote: > My point was that this would be better named Reset() to match other media:: > code. Ah, I am poor at getting the real meaning of the sentence. Will fix. https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... media/base/android/media_codec_bridge.h:85: base::TimeDelta& presentation_time, int* flags); On 2013/02/05 20:24:33, Ami Fischman wrote: > style: no non-const refs in chromium. > s/base::TimeDelta&/base::TimeDelta*/ Done. https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... media/base/android/media_codec_bridge.h:93: size_t PutToInputBuffer(int index, const uint8* data, int size); On 2013/02/05 20:24:33, Ami Fischman wrote: > Why does GIB() still make sense as an API? Wouldn't it make sense to combine > that and this into a single method? > (and, in fact, since the only flag you use is EOS, could drop the magical value > of 0-size buffers and replace with a > bool EnqueueEOS(); I don't think so. we can just call GIB() once after Start(). We don't need to call GIB() every time we feed input data. Maybe, are you talking about combining PTIB() and QIB()? https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... File media/base/android/media_codec_bridge_unittest.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/32001/media/base/android... media/base/android/media_codec_bridge_unittest.cc:151: return; On 2013/02/05 20:24:33, Ami Fischman wrote: > Did you mean to FAIL() here? ah, I mean, depending on vendor's implementation, INFO_OUTPUT_BUFFERS_CHANGED may be returned, but I had to continue here... Changed to continue. Thanks for pointing this out. ;) http://developer.android.com/reference/android/media/MediaCodec.html#INFO_OUT...
https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:184: &AndroidVideoDecodeAccelerator::NotifyEndOfBitstreamBuffer, On 2013/02/07 14:01:36, dwkang1 wrote: > On 2013/02/05 20:24:33, Ami Fischman wrote: > > This is still wrong - > > > https://chromiumcodereview.appspot.com/11973010/diff/22001/content/common/gpu... > > Actually, I implemented the scheme you mentioned (keeping bitstream ids in input > buffers and calling NEOBB when the input buffer id is returned again). However, > it didn't work because the # of input buffers was more than 4 and > kMaxInFlightDecodes in gpu_video_decoder was also 4. gpu_video_decoder didn't > provider bitstream buffer more than four. :( > > BTW, can I ask the background of having "once it is guaranteed that no more > decoded output will be returned > from the contents" semantics? It looks fine to me because the bitstream is > copied when it is passed to the input buffer. Was there any specific use case? The rationale was for knowing how long metadata needed to be maintained. Looking at the .h and the impls, I guess we don't promise the guarantee I phrased above, so I think this is actually ok. https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:357: codec_, gfx::Size(1920, 1080), j_surface.obj()); On 2013/02/07 14:01:36, dwkang1 wrote: > On 2013/02/05 20:24:33, Ami Fischman wrote: > > to ensure we don't hide bugs, can we miminize this (16x16, maybe?)? > I tried from 16x16 ~ 256x256 because 16 ~ 128 showed show some broken area in > the texture. 256 was fine, but now I think using predefined value is not a good > idea. It seems to me that getting the container indicated resolution via vda > could be a resolution. Do you see any problem with this approach? THe only problem with container-specified resolution is that it requires plumbing through the APIs (which span many layers), or else parsing SPS/PPS like the MacVDA does, both of which are grody. Also then you fall victim to containers that lie. https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... File content/common/gpu/media/gles2_external_texture_copier.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/32001/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.cc:38: enum { kVerticeStride = 5 * sizeof(float) }; On 2013/02/05 11:49:36, dwkang1 wrote: > On 2013/02/05 07:43:13, qinmin wrote: > > why enum? > > Done. FWIW, enum has the tiny advantage of forcing compile-time eval and doesn't require storage. https://chromiumcodereview.appspot.com/11973010/diff/43015/content/common/gpu... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/43015/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:135: if (pending_bitstream_buffers_.empty()) On 2013/02/05 08:08:21, dwkang1 wrote: > On 2013/02/05 07:43:13, qinmin wrote: > > no { > > afaik, it is okay to have { if it is consistent in a file in under google3. does > chrome have different rule? No it does not. You can either include braces or not. The Java style guide *does* require braces (even for single-statement clauses), fwiw. https://chromiumcodereview.appspot.com/11973010/diff/43015/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:185: if (bitstream_buffer.id() != -1) { && num_bytes_used_in_the_pending_buffer_ == bitstream_buffer.size() (so really move l.185-189 into the l.170 if? https://chromiumcodereview.appspot.com/11973010/diff/43015/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:229: RETURN_ON_FAILURE(size_ == gfx::Size(width, height), I think you missed my point. If the == is true, then we fall through to the "return" below, but *something* changed out from under us. I think this should be an unconditional failure, and if it triggers in practice we need to dig in and find out what kind of changes are happening and so whether we need to code for them. https://chromiumcodereview.appspot.com/11973010/diff/43015/content/common/gpu... File content/common/gpu/media/gles2_external_texture_copier.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/43015/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.cc:89: if (!vertexShader) return 0; This is counter to the chromium style guide which states: Always linebreak after a conditional, even if the body is only a return or other simple action. (here and below) https://chromiumcodereview.appspot.com/11973010/diff/43015/content/content_te... File content/content_tests.gypi (right): https://chromiumcodereview.appspot.com/11973010/diff/43015/content/content_te... content/content_tests.gypi:893: # TODO(dwkang): make video_decode_accelerator_unittest work on Android. What's the status of this? https://chromiumcodereview.appspot.com/11973010/diff/43015/media/base/android... File media/base/android/media_codec_bridge.h (right): https://chromiumcodereview.appspot.com/11973010/diff/43015/media/base/android... media/base/android/media_codec_bridge.h:92: size_t PutToInputBuffer(int index, const uint8* data, int size); On 2013/02/07 14:01:36, dwkang1 wrote: > On 2013/02/05 20:24:33, Ami Fischman wrote: > > Why does GIB() still make sense as an API? Wouldn't it make sense to combine > > that and this into a single method? > > (and, in fact, since the only flag you use is EOS, could drop the magical > value > > of 0-size buffers and replace with a > > bool EnqueueEOS(); > > I don't think so. we can just call GIB() once after Start(). We don't need to > call GIB() every time we feed input data. > Maybe, are you talking about combining PTIB() and QIB()? Yes, that's what I meant.
https://chromiumcodereview.appspot.com/11973010/diff/43015/content/common/gpu... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/43015/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:135: if (pending_bitstream_buffers_.empty()) On 2013/02/07 19:40:16, Ami Fischman wrote: > On 2013/02/05 08:08:21, dwkang1 wrote: > > On 2013/02/05 07:43:13, qinmin wrote: > > > no { > > > > afaik, it is okay to have { if it is consistent in a file in under google3. > does > > chrome have different rule? > > No it does not. You can either include braces or not. > The Java style guide *does* require braces (even for single-statement clauses), > fwiw. Thanks for the explanation. https://chromiumcodereview.appspot.com/11973010/diff/43015/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:185: if (bitstream_buffer.id() != -1) { On 2013/02/07 19:40:16, Ami Fischman wrote: > && num_bytes_used_in_the_pending_buffer_ == bitstream_buffer.size() > > (so really move l.185-189 into the l.170 if? Correct. will fix in the next patch set. https://chromiumcodereview.appspot.com/11973010/diff/43015/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:229: RETURN_ON_FAILURE(size_ == gfx::Size(width, height), On 2013/02/07 19:40:16, Ami Fischman wrote: > I think you missed my point. If the == is true, then we fall through to the > "return" below, but *something* changed out from under us. I think this should > be an unconditional failure, and if it triggers in practice we need to dig in > and find out what kind of changes are happening and so whether we need to code > for them. Agreed with you. But I realized that this should pass because of the special handling for buggy device in Reset(). I've added a comment. https://chromiumcodereview.appspot.com/11973010/diff/43015/content/common/gpu... File content/common/gpu/media/gles2_external_texture_copier.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/43015/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.cc:89: if (!vertexShader) return 0; On 2013/02/07 19:40:16, Ami Fischman wrote: > This is counter to the chromium style guide which states: > > Always linebreak after a conditional, even if the body is only a return or other > simple action. > > (here and below) Done. https://chromiumcodereview.appspot.com/11973010/diff/43015/content/content_te... File content/content_tests.gypi (right): https://chromiumcodereview.appspot.com/11973010/diff/43015/content/content_te... content/content_tests.gypi:893: # TODO(dwkang): make video_decode_accelerator_unittest work on Android. On 2013/02/07 19:40:16, Ami Fischman wrote: > What's the status of this? This is being addressed by felipe (Clank team). I'll keep sharing the meeting note with you. https://chromiumcodereview.appspot.com/11973010/diff/43015/media/base/android... File media/base/android/media_codec_bridge.h (right): https://chromiumcodereview.appspot.com/11973010/diff/43015/media/base/android... media/base/android/media_codec_bridge.h:92: size_t PutToInputBuffer(int index, const uint8* data, int size); On 2013/02/07 19:40:16, Ami Fischman wrote: > On 2013/02/07 14:01:36, dwkang1 wrote: > > On 2013/02/05 20:24:33, Ami Fischman wrote: > > > Why does GIB() still make sense as an API? Wouldn't it make sense to > combine > > > that and this into a single method? > > > (and, in fact, since the only flag you use is EOS, could drop the magical > > value > > > of 0-size buffers and replace with a > > > bool EnqueueEOS(); > > > > I don't think so. we can just call GIB() once after Start(). We don't need to > > call GIB() every time we feed input data. > > Maybe, are you talking about combining PTIB() and QIB()? > > Yes, that's what I meant. Done.
FYI, the changes on gl related headers will be submitted with a separate change. https://chromiumcodereview.appspot.com/12258009/
Something went wrong with the upload of several files in this CL (PS#8); I suspect you fell for https://code.google.com/p/rietveld/issues/detail?id=357 https://chromiumcodereview.appspot.com/11973010/diff/43015/content/common/gpu... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/43015/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:20: // XXX: apply the scheme for GL access. http://crbug.com/169433 Yay https://chromiumcodereview.appspot.com/11973010/diff/50001/content/common/gpu... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/50001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:27: // XXX: drop the below before submitting. Time to drop this? https://chromiumcodereview.appspot.com/11973010/diff/50001/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:46: // tuned when we have actual use case. Point at http://crbug.com/176036 (which I just filed)? https://chromiumcodereview.appspot.com/11973010/diff/50001/content/common/gpu... File content/common/gpu/media/gles2_external_texture_copier.h (right): https://chromiumcodereview.appspot.com/11973010/diff/50001/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.h:16: class Gles2ExternalTextureCopier { What's the status of using glCopyTextureChromium? https://chromiumcodereview.appspot.com/11973010/diff/50001/media/base/android... File media/base/android/media_codec_bridge.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/50001/media/base/android... media/base/android/media_codec_bridge.cc:220: CHECK(env); unnecessary https://chromiumcodereview.appspot.com/11973010/diff/50001/media/base/android... File media/base/android/media_codec_bridge.h (right): https://chromiumcodereview.appspot.com/11973010/diff/50001/media/base/android... media/base/android/media_codec_bridge.h:67: size_t QueueInputBuffer(int index, const uint8* data, int size, nit: Is there any reason to keep DequeueInputBuffer separate from QIB? IOW ISTM you could drop DIB() entirely, and have QIB() do the dequeue (and return kNoBufferAvailable==-1 on Dequeue failure) and then the Queue. Can be a follow-up, or can not do if you don't like. https://chromiumcodereview.appspot.com/11973010/diff/50001/media/base/android... media/base/android/media_codec_bridge.h:96: enum BufferFlag { nit: can go in the .cc file? https://chromiumcodereview.appspot.com/11973010/diff/50001/media/media.gyp File media/media.gyp (right): https://chromiumcodereview.appspot.com/11973010/diff/50001/media/media.gyp#ne... media/media.gyp:1: 'audio/win/audio_output_win_unittest.cc', Again, upload seems messed up.
Thanks for the review and just uploaded a new patch set to fix the last broken patch set (change is the same). BTW, I have a quick question on your comment. Please have a look. https://chromiumcodereview.appspot.com/11973010/diff/50001/content/common/gpu... File content/common/gpu/media/gles2_external_texture_copier.h (right): https://chromiumcodereview.appspot.com/11973010/diff/50001/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.h:16: class Gles2ExternalTextureCopier { On 2013/02/13 18:07:11, Ami Fischman wrote: > What's the status of using glCopyTextureChromium? I've asked it also to Gregg(gman@), and cced you, but he said glCopyTextureChromium does not support gpu process. Here is part of his response. "glCopyTextureChromium is not implemented for gpu process code. Only renderer process code. Maybe it can be refactored but it won't be called labeled as a GL function like glCopyTextureCHROMIUM it will just be something like GPUCopyTextureToTexture or something." To see the whole mail, you can search for "Regarding OpenGL ES headers" in your mail box. Would it be okay to keep this for a while and use it once we have GPUCopyTextureToTexture? btw, our case is somewhat different from glCopyTextureCHROMIUM because we need to apply transfrom_matrix. What do you think?
https://chromiumcodereview.appspot.com/11973010/diff/50001/content/common/gpu... File content/common/gpu/media/gles2_external_texture_copier.h (right): https://chromiumcodereview.appspot.com/11973010/diff/50001/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.h:16: class Gles2ExternalTextureCopier { On 2013/02/14 01:38:51, dwkang1 wrote: > On 2013/02/13 18:07:11, Ami Fischman wrote: > > What's the status of using glCopyTextureChromium? > > I've asked it also to Gregg(gman@), and cced you, but he said > glCopyTextureChromium does not support gpu process. > > Here is part of his response. > "glCopyTextureChromium is not implemented for gpu process code. Only renderer > process code. Maybe it can be refactored but it won't be called labeled as a GL > function like glCopyTextureCHROMIUM it will just be something like > GPUCopyTextureToTexture or something." > To see the whole mail, you can search for "Regarding OpenGL ES headers" in your > mail box. > > Would it be okay to keep this for a while and use it once we have > GPUCopyTextureToTexture? btw, our case is somewhat different from > glCopyTextureCHROMIUM because we need to apply transfrom_matrix. What do you > think? I remember that thread but thought it ended w/ gman@ offering to expose the copy helper to the GPU-process; I realize on re-reading that it was more tenuous than that so asked a clarifying question on that thread. If gman@ is too busy to do this then I guess we're fine landing the copier helper here, I'd just like to avoid it if possible. Do you know what transform_matrix comes out looking like in practice? My suspicion is that it is either the identity or a pure Y-flip for the decode case (and that in fact it is only used for camera capture rotation/flip purposes, which are inapplicable here).
https://codereview.chromium.org/11973010/diff/50001/content/common/gpu/media/... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/11973010/diff/50001/content/common/gpu/media/... content/common/gpu/media/android_video_decode_accelerator.cc:27: // XXX: drop the below before submitting. On 2013/02/13 18:07:11, Ami Fischman wrote: > Time to drop this? Let me do this once we have a conclusion on gl copy issue. =) https://codereview.chromium.org/11973010/diff/50001/content/common/gpu/media/... content/common/gpu/media/android_video_decode_accelerator.cc:46: // tuned when we have actual use case. On 2013/02/13 18:07:11, Ami Fischman wrote: > Point at http://crbug.com/176036 (which I just filed)? Done. Thanks for filing. https://codereview.chromium.org/11973010/diff/50001/content/common/gpu/media/... File content/common/gpu/media/gles2_external_texture_copier.h (right): https://codereview.chromium.org/11973010/diff/50001/content/common/gpu/media/... content/common/gpu/media/gles2_external_texture_copier.h:16: class Gles2ExternalTextureCopier { On 2013/02/14 16:43:25, Ami Fischman wrote: > On 2013/02/14 01:38:51, dwkang1 wrote: > > On 2013/02/13 18:07:11, Ami Fischman wrote: > > > What's the status of using glCopyTextureChromium? > > > > I've asked it also to Gregg(gman@), and cced you, but he said > > glCopyTextureChromium does not support gpu process. > > > > Here is part of his response. > > "glCopyTextureChromium is not implemented for gpu process code. Only renderer > > process code. Maybe it can be refactored but it won't be called labeled as a > GL > > function like glCopyTextureCHROMIUM it will just be something like > > GPUCopyTextureToTexture or something." > > To see the whole mail, you can search for "Regarding OpenGL ES headers" in > your > > mail box. > > > > Would it be okay to keep this for a while and use it once we have > > GPUCopyTextureToTexture? btw, our case is somewhat different from > > glCopyTextureCHROMIUM because we need to apply transfrom_matrix. What do you > > think? > > I remember that thread but thought it ended w/ gman@ offering to expose the copy > helper to the GPU-process; I realize on re-reading that it was more tenuous than > that so asked a clarifying question on that thread. > > If gman@ is too busy to do this then I guess we're fine landing the copier > helper here, I'd just like to avoid it if possible. > > Do you know what transform_matrix comes out looking like in practice? My > suspicion is that it is either the identity or a pure Y-flip for the decode case > (and that in fact it is only used for camera capture rotation/flip purposes, > which are inapplicable here). Unfortunately, CopyTextureCHROMIUMResourceManager does not support GL_TEXTURE_EXTERNAL_OES. I've asked this on the thread. :( Regarding transform_matrix, it was vflip matrix when I checked with N10 and Xoom. https://cs.corp.google.com/#googletv/frameworks/native/libs/gui/SurfaceTextur... I've checked rotation transform used in MediaPlayer path, but not in MediaCodec path. https://cs.corp.google.com/#googletv/frameworks/av/media/libstagefright/OMXCo... https://codereview.chromium.org/11973010/diff/50001/media/base/android/media_... File media/base/android/media_codec_bridge.cc (right): https://codereview.chromium.org/11973010/diff/50001/media/base/android/media_... media/base/android/media_codec_bridge.cc:220: CHECK(env); On 2013/02/13 18:07:11, Ami Fischman wrote: > unnecessary Done. https://codereview.chromium.org/11973010/diff/50001/media/base/android/media_... File media/base/android/media_codec_bridge.h (right): https://codereview.chromium.org/11973010/diff/50001/media/base/android/media_... media/base/android/media_codec_bridge.h:67: size_t QueueInputBuffer(int index, const uint8* data, int size, On 2013/02/13 18:07:11, Ami Fischman wrote: > nit: Is there any reason to keep DequeueInputBuffer separate from QIB? IOW ISTM > you could drop DIB() entirely, and have QIB() do the dequeue (and return > kNoBufferAvailable==-1 on Dequeue failure) and then the Queue. Can be a > follow-up, or can not do if you don't like. Tried to apply it, but I realized that the current approach is little more efficient because it does not need to prepare the buffer from the bitstream in INFO_TRY_AGAIN_LATER case. If you are not strong on this, I want to keep this. https://codereview.chromium.org/11973010/diff/50001/media/base/android/media_... media/base/android/media_codec_bridge.h:96: enum BufferFlag { On 2013/02/13 18:07:11, Ami Fischman wrote: > nit: can go in the .cc file? Done.
Looking good! You should definitely get a review of the copier from someone who knows GL a lot better than me, though. https://codereview.chromium.org/11973010/diff/71003/content/common/gpu/media/... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/11973010/diff/71003/content/common/gpu/media/... content/common/gpu/media/android_video_decode_accelerator.cc:160: if (bitstream_buffer.size() > 0) { nit: since you moved the .pop for EOS buffers above here, your code here and elsewhere in this file might become simpler if you short-circuit the case of empty buffers in Decode itself (immediately NEOBB and drop the buffer without sending it through the decoder). https://codereview.chromium.org/11973010/diff/71003/content/common/gpu/media/... File content/common/gpu/media/gles2_external_texture_copier.cc (right): https://codereview.chromium.org/11973010/diff/71003/content/common/gpu/media/... content/common/gpu/media/gles2_external_texture_copier.cc:38: static const int kVerticeStride = 5 * sizeof(float); English: The singular of "vertices" is "vertex" ;) I think in this case you want the plural though: s/kVerticeStride/kVerticesStride/ https://codereview.chromium.org/11973010/diff/71003/content/common/gpu/media/... content/common/gpu/media/gles2_external_texture_copier.cc:53: static const GLfloat g_mvp_matrix[] = { My glFoo is super-weak, but I believe what you have is equivalent to: - drop uMVPMatrix (just use gl_position = aPosition in vertex shader) - switch the 0's and 1's in the T coordinates in g_vertices; IOW the "Y" in texture-coords. This would save you l.47-59, I think. https://codereview.chromium.org/11973010/diff/71003/content/common/gpu/media/... content/common/gpu/media/gles2_external_texture_copier.cc:60: static GLuint LoadShader(GLenum shaderType, const char* pSource) { here and elsewhere, unix_style, not camelCase for variables & parameters. (isn't it fun to work in both Java & C++ at the same time?!??!?) https://codereview.chromium.org/11973010/diff/71003/content/common/gpu/media/... content/common/gpu/media/gles2_external_texture_copier.cc:62: if (shader) { nit: if you reversed the test you could early-return and de-indent the rest of the function. Ditto for similar cases below. https://codereview.chromium.org/11973010/diff/71003/content/common/gpu/media/... content/common/gpu/media/gles2_external_texture_copier.cc:71: char* buf = (char*) malloc(infoLen); scoped_ptr https://codereview.chromium.org/11973010/diff/71003/content/common/gpu/media/... content/common/gpu/media/gles2_external_texture_copier.cc:72: if (buf) { unnecessary; malloc/new never fail in chromium other than by crashing the process. https://codereview.chromium.org/11973010/diff/71003/content/common/gpu/media/... content/common/gpu/media/gles2_external_texture_copier.cc:91: GLuint pixelShader = LoadShader(GL_FRAGMENT_SHADER, pFragmentSource); s/pixel/fragment/ ? (here and below) https://codereview.chromium.org/11973010/diff/71003/content/common/gpu/media/... content/common/gpu/media/gles2_external_texture_copier.cc:97: glAttachShader(program, vertexShader); delete the shaders after attaching them (so program has the only reference) https://codereview.chromium.org/11973010/diff/71003/content/common/gpu/media/... content/common/gpu/media/gles2_external_texture_copier.cc:168: glViewport(0, 0, w, h); I suspect this can go in a Setup method https://codereview.chromium.org/11973010/diff/71003/content/common/gpu/media/... content/common/gpu/media/gles2_external_texture_copier.cc:171: glClear(GL_DEPTH_BUFFER_BIT | GL_COLOR_BUFFER_BIT); why DEPTH? (IOW, why allocate a depthbuffer later?) https://codereview.chromium.org/11973010/diff/71003/content/common/gpu/media/... content/common/gpu/media/gles2_external_texture_copier.cc:181: glVertexAttribPointer( l.181-195 can probably be done only once, right? (when program_ is created) https://codereview.chromium.org/11973010/diff/71003/content/common/gpu/media/... content/common/gpu/media/gles2_external_texture_copier.cc:196: glUniformMatrix4fv(st_matrix_handle_, 1, GL_FALSE, transfrom_matrix); Part of me wants to just ignore transform_matrix and assume it's always a vflip... :( https://codereview.chromium.org/11973010/diff/71003/content/common/gpu/media/... content/common/gpu/media/gles2_external_texture_copier.cc:205: LOG(ERROR) << "Init should not be called twice."; Since this is the only failure mode of Init(), I think it's confusing to make it a separate API point. Why not just inline it into the ctor? But perhaps you meant to not discard the bool return values form the Setup* functions? https://codereview.chromium.org/11973010/diff/71003/content/common/gpu/media/... File content/common/gpu/media/gles2_external_texture_copier.h (right): https://codereview.chromium.org/11973010/diff/71003/content/common/gpu/media/... content/common/gpu/media/gles2_external_texture_copier.h:30: void RenderFrame(int32 width, int32 height, GLuint texture_id, This is a weird place to sort this function into, considering the previous one and the following 3 are all part of Init ;) I guess I'd just inline it into Copy(). https://codereview.chromium.org/11973010/diff/71003/content/common/gpu/media/... content/common/gpu/media/gles2_external_texture_copier.h:30: void RenderFrame(int32 width, int32 height, GLuint texture_id, what does it mean if width/height here are different from Init()'s args? Need doco. https://codereview.chromium.org/11973010/diff/71003/content/common/gpu/media/... content/common/gpu/media/gles2_external_texture_copier.h:34: void SaveState(); Replace with ScopedFrameBufferBinder / ScopedTextureBinder ?
Thanks for the review Ami. I've added some throttling logic in android_video_decoder_accelerator code. Please have a look. Regarding the comments on copier, let me address them in the next patch set. (plan is to try to use CopyTextureCHROMIUMResourceManager after making it support GL_TEXTURE_EXTERNAL_OES.) Thanks, https://codereview.chromium.org/11973010/diff/71003/content/common/gpu/media/... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/11973010/diff/71003/content/common/gpu/media/... content/common/gpu/media/android_video_decode_accelerator.cc:160: if (bitstream_buffer.size() > 0) { On 2013/02/17 00:12:44, Ami Fischman wrote: > nit: since you moved the .pop for EOS buffers above here, your code here and > elsewhere in this file might become simpler if you short-circuit the case of > empty buffers in Decode itself (immediately NEOBB and drop the buffer without > sending it through the decoder). Good idea. Thanks!
https://codereview.chromium.org/11973010/diff/80002/content/common/gpu/media/... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/11973010/diff/80002/content/common/gpu/media/... content/common/gpu/media/android_video_decode_accelerator.cc:140: if (bitstreams_notified_in_advance_.size() > kMaxBitstreamsNotifiedInAdvance) Why do you need this business? Isn't it best to saturate the decoder, which you already get by handling INFO_TRY_AGAIN_LATER from DequeueInputBuffer below? Put another way, how can this test become true? Does MediaCodec do its own internal buffering of input and immediately free the (public API) inputbuffers as soon as QueueInputBuffer returns? https://codereview.chromium.org/11973010/diff/80002/content/common/gpu/media/... content/common/gpu/media/android_video_decode_accelerator.cc:324: if (bitstream_buffer.id() != -1 && bitstream_buffer.size() == 0) { why would id be -1 here?
https://codereview.chromium.org/11973010/diff/80002/content/common/gpu/media/... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/11973010/diff/80002/content/common/gpu/media/... content/common/gpu/media/android_video_decode_accelerator.cc:140: if (bitstreams_notified_in_advance_.size() > kMaxBitstreamsNotifiedInAdvance) On 2013/02/18 17:46:48, Ami Fischman wrote: > Why do you need this business? > Isn't it best to saturate the decoder, which you already get by handling > INFO_TRY_AGAIN_LATER from DequeueInputBuffer below? > > Put another way, how can this test become true? Does MediaCodec do its own > internal buffering of input and immediately free the (public API) inputbuffers > as soon as QueueInputBuffer returns? Yes. To be specific, internal buffer lived in the openmax decoder component, which is in the MediaCodec. The reason I added this logic was one implementation I tested had a large internal buffer and immediately free the input buffers by calling *EmptyBufferDone*. So, first, I thought we should fix their implementation, but it turned out that the behavior was okay with the IL spec. I've checked this with Erdi, who is OpenMax IL spec editor. I'll forward the mail to you. Thanks, https://codereview.chromium.org/11973010/diff/80002/content/common/gpu/media/... content/common/gpu/media/android_video_decode_accelerator.cc:324: if (bitstream_buffer.id() != -1 && bitstream_buffer.size() == 0) { On 2013/02/18 17:46:48, Ami Fischman wrote: > why would id be -1 here? I added this test because Flush() calls this.
https://codereview.chromium.org/11973010/diff/80002/content/common/gpu/media/... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/11973010/diff/80002/content/common/gpu/media/... content/common/gpu/media/android_video_decode_accelerator.cc:140: if (bitstreams_notified_in_advance_.size() > kMaxBitstreamsNotifiedInAdvance) On 2013/02/19 02:14:55, dwkang1 wrote: > On 2013/02/18 17:46:48, Ami Fischman wrote: > > Why do you need this business? > > Isn't it best to saturate the decoder, which you already get by handling > > INFO_TRY_AGAIN_LATER from DequeueInputBuffer below? > > > > Put another way, how can this test become true? Does MediaCodec do its own > > internal buffering of input and immediately free the (public API) inputbuffers > > as soon as QueueInputBuffer returns? > > Yes. To be specific, internal buffer lived in the openmax decoder component, > which is in the MediaCodec. The reason I added this logic was one implementation > I tested had a large internal buffer and immediately free the input buffers by > calling *EmptyBufferDone*. So, first, I thought we should fix their > implementation, but it turned out that the behavior was okay with the IL spec. > I've checked this with Erdi, who is OpenMax IL spec editor. I'll forward the > mail to you. Thanks, Hmm. That reading of the spec makes NEOBB pretty much useless in the VDA API (since we leave PTS outside that API, meaning only VDA clients can make the determination of bitstreambuffers being entirely consumed). I guess I'm inclined to burn NEOBB. Obviously, that's independent of this CL :)
Hi Gregg and John, I've added you for changes related to GL. Could you take a look? Thanks!
https://chromiumcodereview.appspot.com/11973010/diff/87001/media/base/android... File media/base/android/media_codec_bridge.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/87001/media/base/android... media/base/android/media_codec_bridge.cc:270: JNI_MediaCodec::Java_MediaCodec_releaseOutputBuffer( I thought that before calling releaseOutputBuffer you need to call clear() method on the ByteBuffer object. Something like: ScopedJavaLocalRef<jobject> j_buffer(env, env->GetObjectArrayElement(j_output_buffers_.obj(), index)); JNI_ByteBuffer::Java_ByteBuffer_clear(env, j_buffer_.obj());
https://chromiumcodereview.appspot.com/11973010/diff/87001/content/common/gpu... File content/common/gpu/media/gles2_external_texture_copier.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/87001/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.cc:22: : width_(width), style nit: 4 spaces in front of : https://chromiumcodereview.appspot.com/11973010/diff/87001/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.cc:49: glGetIntegerv(GL_RENDERBUFFER_BINDING, &previous_renderbuffer_id_); render buffers are not used by copy https://chromiumcodereview.appspot.com/11973010/diff/87001/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.cc:50: glGetIntegerv(GL_TEXTURE_BINDING_2D, &previous_texture_2d_id_); CopyTextureCHROMIUMResourceManager::DoCopyTexture calls glActiveTexture and uses texture unit 0. That means you need to query the active texture unit. You then need to call glActiveTexture(GL_TEXTURE0) before you query the GL_TEXTURE_BINDING_2D query. You'll also need to restore in the opposite order. First restore the binding then restore the active texture unit back to the unit it was at https://chromiumcodereview.appspot.com/11973010/diff/87001/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.cc:55: You also need to query/restore GL_DEPTH_TEST, GL_SCISSOR_TEST, GL_STENCIL_TEST, GL_CULL_FACE, glColorMask, glDepthMask, GL_BLEND and you need to query https://chromiumcodereview.appspot.com/11973010/diff/87001/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.cc:60: CheckGlError("SaveState"); You also need to query and restore the Texture Parameters GL_TEXTURE_WRAP_S, GL_TEXTURE_WRAP_T, GL_TEXTURE_MAG_FILTER, and GL_TEXTURE_MIN_FILTER for your source and dest textures. https://chromiumcodereview.appspot.com/11973010/diff/87001/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.cc:67: glUseProgram(previous_program_); Note: Querying and restoring bindings can never work correctly. The reason is resources are ref counted. So, if any of these resources were deleted in another context their only reference is their binding in this context. When they get unbound, by binding something new in CopyTextureCHROMIUMResourceManager::DoCopyTexture, they are released and these bindings trying to restore them will fail. Yet another reason why query and restore does not work in GL.
I know Gregg's changes are on the way. https://codereview.chromium.org/12321068/ https://codereview.chromium.org/12315051/ But, I uploaded this change in order to get reviews for the changes on AVDA and gpu/... I'll rebase and integrate it once they are submitted. PTAL. Thanks! https://chromiumcodereview.appspot.com/11973010/diff/87001/content/common/gpu... File content/common/gpu/media/gles2_external_texture_copier.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/87001/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.cc:22: : width_(width), On 2013/02/20 19:13:58, greggman wrote: > style nit: 4 spaces in front of : Done. https://chromiumcodereview.appspot.com/11973010/diff/87001/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.cc:49: glGetIntegerv(GL_RENDERBUFFER_BINDING, &previous_renderbuffer_id_); On 2013/02/20 19:13:58, greggman wrote: > render buffers are not used by copy Done. https://chromiumcodereview.appspot.com/11973010/diff/87001/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.cc:50: glGetIntegerv(GL_TEXTURE_BINDING_2D, &previous_texture_2d_id_); On 2013/02/20 19:13:58, greggman wrote: > CopyTextureCHROMIUMResourceManager::DoCopyTexture calls glActiveTexture and uses > texture unit 0. That means you need to query the active texture unit. You then > need to call glActiveTexture(GL_TEXTURE0) before you query the > GL_TEXTURE_BINDING_2D query. You'll also need to restore in the opposite order. > First restore the binding then restore the active texture unit back to the unit > it was at Done. https://chromiumcodereview.appspot.com/11973010/diff/87001/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.cc:55: On 2013/02/20 19:13:58, greggman wrote: > You also need to query/restore GL_DEPTH_TEST, GL_SCISSOR_TEST, GL_STENCIL_TEST, > GL_CULL_FACE, glColorMask, glDepthMask, GL_BLEND and you need to query Done. https://chromiumcodereview.appspot.com/11973010/diff/87001/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.cc:60: CheckGlError("SaveState"); On 2013/02/20 19:13:58, greggman wrote: > You also need to query and restore the Texture Parameters GL_TEXTURE_WRAP_S, > GL_TEXTURE_WRAP_T, GL_TEXTURE_MAG_FILTER, and GL_TEXTURE_MIN_FILTER for your > source and dest textures. The doc says glTexParameter sets params for the active texture unit. So, I believe they should be restored when we restore the state of GL_TEXTURE0. Please correct me if I am wrong. http://www.opengl.org/sdk/docs/man/xhtml/glTexParameter.xml "glTexParameter specifies the texture parameters for the active texture unit, specified by calling glActiveTexture." https://chromiumcodereview.appspot.com/11973010/diff/87001/content/common/gpu... content/common/gpu/media/gles2_external_texture_copier.cc:67: glUseProgram(previous_program_); On 2013/02/20 19:13:58, greggman wrote: > Note: Querying and restoring bindings can never work correctly. The reason is > resources are ref counted. So, if any of these resources were deleted in another > context their only reference is their binding in this context. When they get > unbound, by binding something new in > CopyTextureCHROMIUMResourceManager::DoCopyTexture, they are released and these > bindings trying to restore them will fail. > > Yet another reason why query and restore does not work in GL. I added a TODO message for it. https://chromiumcodereview.appspot.com/11973010/diff/87001/media/base/android... File media/base/android/media_codec_bridge.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/87001/media/base/android... media/base/android/media_codec_bridge.cc:270: JNI_MediaCodec::Java_MediaCodec_releaseOutputBuffer( On 2013/02/20 18:35:26, felipeg1 wrote: > I thought that before calling releaseOutputBuffer you need to call clear() > method on the ByteBuffer object. > Something like: > > ScopedJavaLocalRef<jobject> j_buffer(env, > env->GetObjectArrayElement(j_output_buffers_.obj(), index)); > JNI_ByteBuffer::Java_ByteBuffer_clear(env, j_buffer_.obj()); > > Actually, we are assuming that the ByteBuffer is DirectBuffer and the offset & size are passed via BufferInfo. Do you see any problem with not calling clear()?
AVDA.{h,cc} diffs PS13 -> PS17 LGTM.
Hi Gregg and Ami, It looks like 12315051 will stay in the CQ today. So, I just patched 12315051 in the patch set 18 and make a revision in order to get early review. To see the real change please see the diff between 18 and 19. Thanks!
PS18->PS19 LGTM
Just a minor thing, otherwise lgtm https://codereview.chromium.org/11973010/diff/106001/content/common/gpu/media... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/11973010/diff/106001/content/common/gpu/media... content/common/gpu/media/android_video_decode_accelerator.cc:315: gl_decoder_->RestoreFramebufferBindings(); This store is not needed? I made Initialize take a decoder so it can restore state.
https://codereview.chromium.org/11973010/diff/106001/content/common/gpu/media... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/11973010/diff/106001/content/common/gpu/media... content/common/gpu/media/android_video_decode_accelerator.cc:315: gl_decoder_->RestoreFramebufferBindings(); On 2013/02/28 00:39:27, greggman wrote: > This store is not needed? I made Initialize take a decoder so it can restore > state. I just checked your patch set 2. ;-) I'll remove it in the next patch set. Thanks!
Ami, actually, I've added a modification for api version checking in patch set 20. Could you take a look? (I will upload a new patch set soon once Gregg's patch is available in git.)
Rebased and applied Gregg's comment. PTAL. https://chromiumcodereview.appspot.com/11973010/diff/106001/content/common/gp... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/106001/content/common/gp... content/common/gpu/media/android_video_decode_accelerator.cc:315: gl_decoder_->RestoreFramebufferBindings(); On 2013/02/28 00:45:25, dwkang1 wrote: > On 2013/02/28 00:39:27, greggman wrote: > > This store is not needed? I made Initialize take a decoder so it can restore > > state. > I just checked your patch set 2. ;-) I'll remove it in the next patch set. > Thanks! Done.
19->20 & 21->22 LGTM https://codereview.chromium.org/11973010/diff/97004/content/common/gpu/media/... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/11973010/diff/97004/content/common/gpu/media/... content/common/gpu/media/android_video_decode_accelerator.cc:310: if (!copier_.get()) { Doesn't scoped_ptr<>::operator*() mean you don't need the .get() here? https://codereview.chromium.org/11973010/diff/97004/content/common/gpu/media/... content/common/gpu/media/android_video_decode_accelerator.cc:445: if (copier_.get()) ditto https://codereview.chromium.org/11973010/diff/106001/media/base/android/media... File media/base/android/media_codec_bridge_unittest.cc (right): https://codereview.chromium.org/11973010/diff/106001/media/base/android/media... media/base/android/media_codec_bridge_unittest.cc:96: if (!MediaCodecBridge::IsAvailable()) There are test bots that run <JB?
Thanks a lot for your thoughtful and kind reviewing! I've learned a lot from you. https://chromiumcodereview.appspot.com/11973010/diff/97004/content/common/gpu... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/97004/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:310: if (!copier_.get()) { On 2013/02/28 03:23:05, Ami Fischman wrote: > Doesn't scoped_ptr<>::operator*() mean you don't need the .get() here? No. Fixed. https://chromiumcodereview.appspot.com/11973010/diff/97004/content/common/gpu... content/common/gpu/media/android_video_decode_accelerator.cc:445: if (copier_.get()) On 2013/02/28 03:23:05, Ami Fischman wrote: > ditto Done. https://chromiumcodereview.appspot.com/11973010/diff/106001/media/base/androi... File media/base/android/media_codec_bridge_unittest.cc (right): https://chromiumcodereview.appspot.com/11973010/diff/106001/media/base/androi... media/base/android/media_codec_bridge_unittest.cc:96: if (!MediaCodecBridge::IsAvailable()) On 2013/02/28 03:23:05, Ami Fischman wrote: > There are test bots that run <JB? Not sure, but I added this because minSDK was 14 in AndroidMenifest.xml of Chrome for android and media_unittests apk.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dwkang@chromium.org/11973010/112005
Presubmit check for 11973010-112005 failed and returned exit status 1.
INFO:root:Found 12 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
content/content_tests.gypi
content/content_common.gypi
Presubmit checks took 2.3s to calculate.
Hi Xiaohan,
I've added you as a reviewer for:
content/content_tests.gypi
content/content_common.gypi
Could you take a look?
Thanks!
lgtm % nits https://chromiumcodereview.appspot.com/11973010/diff/112005/content/content_t... File content/content_tests.gypi (right): https://chromiumcodereview.appspot.com/11973010/diff/112005/content/content_t... content/content_tests.gypi:900: # TODO(felipeg): make video_decode_accelerator_unittest work on Android. nit: s/make/Make https://chromiumcodereview.appspot.com/11973010/diff/112005/content/content_t... content/content_tests.gypi:901: # crbug.com/178647 nit: We don't usually use spaces to do alignment for TODO. Please see other TODOs in this file for examples. Also, use full URL: http://crbug.com/178647
On 2013/02/28 21:19:11, xhwang wrote: > lgtm % nits > > https://chromiumcodereview.appspot.com/11973010/diff/112005/content/content_t... > File content/content_tests.gypi (right): > > https://chromiumcodereview.appspot.com/11973010/diff/112005/content/content_t... > content/content_tests.gypi:900: # TODO(felipeg): make > video_decode_accelerator_unittest work on Android. > nit: s/make/Make > > https://chromiumcodereview.appspot.com/11973010/diff/112005/content/content_t... > content/content_tests.gypi:901: # crbug.com/178647 > nit: We don't usually use spaces to do alignment for TODO. Please see other > TODOs in this file for examples. > > Also, use full URL: http://crbug.com/178647 BTW, I am not an OWNER for these two files. You need to find an OWNER to get approval (see src/content/OWNERS).
Thanks for the review. btw, I still see the : presubmit messages for content_xxx.gyp files. even though I added xhwang who is one of the suggested owners when I uploaded the previous patch set. https://chromiumcodereview.appspot.com/11973010/diff/112005/content/content_t... File content/content_tests.gypi (right): https://chromiumcodereview.appspot.com/11973010/diff/112005/content/content_t... content/content_tests.gypi:900: # TODO(felipeg): make video_decode_accelerator_unittest work on Android. On 2013/02/28 21:19:11, xhwang wrote: > nit: s/make/Make Done. https://chromiumcodereview.appspot.com/11973010/diff/112005/content/content_t... content/content_tests.gypi:901: # crbug.com/178647 On 2013/02/28 21:19:11, xhwang wrote: > nit: We don't usually use spaces to do alignment for TODO. Please see other > TODOs in this file for examples. > > Also, use full URL: http://crbug.com/178647 Done.
I see. Then it seems that it was a presubmit bug? Let me add a owner from content/OWNERS file. Thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dwkang@chromium.org/11973010/119002
Presubmit check for 11973010-119002 failed and returned exit status 1.
INFO:root:Found 12 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
content/content_tests.gypi
content/content_common.gypi
Presubmit checks took 2.9s to calculate.
Hi Antoine(piman), Would you take a look at this change? (for contents_xxx.gypi flies) Thanks!
I didn't look at the rest of the CL. One comment about the gyp https://chromiumcodereview.appspot.com/11973010/diff/119002/content/content_c... File content/content_common.gypi (right): https://chromiumcodereview.appspot.com/11973010/diff/119002/content/content_c... content/content_common.gypi:501: '-lGLESv2', This is rather unusual, we shouldn't be linking directly against -lEGL or -lGLESv2, but go through the bindings - even on Android - by adding the ui/gl/gl.gyp:gl dependency (which should already be there anyway).
Thanks for the prompt review. PTAL. https://chromiumcodereview.appspot.com/11973010/diff/119002/content/content_c... File content/content_common.gypi (right): https://chromiumcodereview.appspot.com/11973010/diff/119002/content/content_c... content/content_common.gypi:501: '-lGLESv2', On 2013/02/28 22:02:56, piman wrote: > This is rather unusual, we shouldn't be linking directly against -lEGL or > -lGLESv2, but go through the bindings - even on Android - by adding the > ui/gl/gl.gyp:gl dependency (which should already be there anyway). Removed.
lgtm
https://chromiumcodereview.appspot.com/11973010/diff/119002/content/content_c... File content/content_common.gypi (right): https://chromiumcodereview.appspot.com/11973010/diff/119002/content/content_c... content/content_common.gypi:501: '-lGLESv2', On 2013/02/28 22:36:28, dwkang1 wrote: > On 2013/02/28 22:02:56, piman wrote: > > This is rather unusual, we shouldn't be linking directly against -lEGL or > > -lGLESv2, but go through the bindings - even on Android - by adding the > > ui/gl/gl.gyp:gl dependency (which should already be there anyway). > > Removed. Oh, I misunderstood your comment. ui/gl/gl.gyp:gl dependency is added.
On 2013/02/28 22:55:42, dwkang1 wrote: > https://chromiumcodereview.appspot.com/11973010/diff/119002/content/content_c... > File content/content_common.gypi (right): > > https://chromiumcodereview.appspot.com/11973010/diff/119002/content/content_c... > content/content_common.gypi:501: '-lGLESv2', > On 2013/02/28 22:36:28, dwkang1 wrote: > > On 2013/02/28 22:02:56, piman wrote: > > > This is rather unusual, we shouldn't be linking directly against -lEGL or > > > -lGLESv2, but go through the bindings - even on Android - by adding the > > > ui/gl/gl.gyp:gl dependency (which should already be there anyway). > > > > Removed. > Oh, I misunderstood your comment. ui/gl/gl.gyp:gl dependency is added. It's already there (l.428), so no need to add it again.
Last patch set is reverted. Thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dwkang@chromium.org/11973010/115012
Sorry for I got bad news for ya. Compile failed with a clobber build on android_clang_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dwkang@chromium.org/11973010/108025
Message was sent while issue was closed.
Change committed as 185469
Hi All Does anyone knows how to handle video frame which comes out with flag : BUFFER_FLAG_END_OF_STREAM ? render it or just drop it ? I cannot get enough info from Google website. Android MediaCodec. Thanks. 在 2013年3月1日星期五UTC+8下午12时59分00秒，commi...@chromium.org写道： > > Change committed as 185469 > > https://chromiumcodereview.appspot.com/11973010/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
And I only can get video output buffer time stamp and flag, can not get others info: size and offset. Thanks. 2014-09-05 12:48 GMT+08:00 <dssxkk@gmail.com>: > > Hi All > > Does anyone knows how to handle video frame which comes out with flag > : BUFFER_FLAG_END_OF_STREAM ? render it or just drop it ? I cannot get > enough info from Google website. > > Android MediaCodec. > > Thanks. > > > 在 2013年3月1日星期五UTC+8下午12时59分00秒，commi...@chromium.org写道： >> >> Change committed as 185469 >> >> https://chromiumcodereview.appspot.com/11973010/ >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
