Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14914009:
VAVDA: Redesign stage 1. (Closed)
|
Created:
7 years, 7 months ago by Pawel Osciak Modified:
7 years, 6 months ago Reviewers:
posciak1, Ami GONE FROM CHROMIUM, piman, commit-bot: I haz the power, marcheu, scherkus (not reviewing) CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, apatrick_chromium, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionVAVDA: Redesign stage 1.
This is the first stage of VAVDA VaapiH264Decoder redesigns, aimed at
simplifying code, improving and clarifying resource management and lifecycles,
reducing dependencies and unnecessary complexity, removing locking, simplifying
state machines and reducing threading dependencies, as well as to prepare for
new features, including missing frame concealment and dynamic resolution
changes.
The second stage will involve simplification and hopefully removal of locking
in VAVDA itself.
Main changes:
- Introduce VaapiDelegate class to handle all VAAPI calls and libva locking,
as well as manage VA resources.
- VaapiH264Decoder now runs entirely on one thread. No more confusing rules
which methods need to be run on the ChildThread.
- VaapiH264Decoder is now oblivious to GLX, textures, pixmaps, etc. and
only uses VA surfaces. VAVDA now manages the rest.
- VA surfaces used for decoding are decoupled from TFP pictures. Also,
otuput buffers passed to/from client are no longer tied to VA surfaces and
don't go to Decoder, simplifying things and squeezing some more parallelism.
- Most resources are now automatically managed instead of having to manage them
explicitly.
- Remove confusing decode surface states in favor of automatic refcounting
and releasing.
- Perform texture to pixmap binding on each frame to conform with the TFP
spec (this is a no-op on our CrOS platforms, so it didn't matter
correctness-wise, neither does this affect performance)
- Simplify VaapiH264Decoder in many places, including a simpler state
machine.
- Extracted some more parallelism in places.
- And more...
Performance is comparable (within measurement error) to old VAVDA, although
I have seen FPS gains of ~10-20% in some situations on specific streams and
during multiple simultaneous decodes.
BUG=222427, 177422, 151045
TEST=vda unittest, videotestmatrix, vimeo, youtube, timescapes, seek tests,
memory leak tests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203343
Patch Set 1 : #
Total comments: 185
Patch Set 2 : #
Total comments: 24
Patch Set 3 : #
Total comments: 4
Patch Set 4 : Added DCHECKs for ChildThread in TFPPicture #
Total comments: 7
Patch Set 5 : Remove glTexParameterf calls and va_surface_id() in favor of va_surface()->id() #Patch Set 6 : #Patch Set 7 : Rebase #Patch Set 8 : Add CONTENT_EXPORT to VaapiWrapper #Messages
Total messages: 38 (0 generated)
PTAL.
On 2013/05/10 23:22:11, posciak wrote: > PTAL. Dear reviewers, I would really appreciate a quick look please. Thanks!
On Fri, May 17, 2013 at 7:32 AM, <posciak@chromium.org> wrote: > Dear reviewers, I would really appreciate a quick look please. Thanks! > lol, "quick". I've had this review open in my browser for a while, and making slow progress on it.
This is a significant improvement over the previous version of the code! https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... File content/common/gpu/media/vaapi_delegate.cc (right): https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.cc:11: #define LOG_VA_ERROR_AND_REPORT(va_res, err_msg) \ s/va_res/va_error/ here and in the next 4 lines https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.cc:35: void *vaapi_x11_handle = NULL; static (this & above) https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.cc:106: #define VAAPI_SYM(name, handle) Vaapi##name VAAPI_##name = NULL #undef after done w/ these? https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.cc:126: VAAPI_SYM(ErrorStr, vaapi_handle); Any reason not to do these (and the typedefs above and the dlsym's at the bottom of the file) in sorted order? https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.cc:130: VAProfile& va_profile) { non-const ref https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.cc:138: case media::H264PROFILE_HIGH: What about the variants of HIGH in media? Do they not get handled by VA's "high"? (also, does using VA "high" for media's EXTENDED not work?) https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.cc:173: scoped_refptr<VaapiDelegate> vaapi_delegate(new VaapiDelegate); nit: never omit the parens on a new statement. https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.cc:184: static bool vaapi_functions_initialized = PostSandboxInitialization(); This idiom is not thread-safe. (https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/p6h3HC8Wro4...) https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.cc:212: VAConfigAttrib attrib; s/;/ = {0};/ ? https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.cc:215: VAEntrypoint entrypoint = VAEntrypointVLD; necessary? (at the very least, should be const & name begin with k) https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.cc:273: DestroySurfaces(); I think this isn't really necessary, in which case inline DestroySurfaces into the dtor. https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.cc:278: va_surfaces.push_back(va_surface_ids_[i]); Once the member is a vector too you can simply assign the vector here (not member-wise). https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.cc:351: std::copy(pending_va_bufs_.begin(), pending_va_bufs_.end(), va_buffers.get()); Why the copies? Is VAAPI_RenderPicture destructive of its arguments? Or can you instead simply use &pending_va_bufs_[0] for the param? https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.cc:356: slice_buffers.get()); ditto https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.cc:381: bool result = SubmitDecode(va_surface_id); The lock is not held across this & the next line, meaning that one thread's DPB() call may nuke another thread's just-submitted DADPB() call? Put another way, I suspect a better layout would be to rename DestroyPendingBuffers to DestroyPendingBuffers_Locked, make DestroyPendingBuffers acquire the lock and call DestroyPendingBuffers_Locked(), inline SubmitDecode into DADPB(), and call DestroyPendingBuffers_Locked() from the bottom of DADPB. https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.cc:399: 0, 0, width, height, is there an assumption that this class only knows about coded sizes, but not visible rect or padding issues? https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.cc:445: return VAAPI_GetDisplay && FWIW you could test for NULL and early-return as part of the macro, which would also let you emit a more useful log message (about which symbol failed to load). https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... File content/common/gpu/media/vaapi_delegate.h (right): https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.h:21: // A VA-API-specific decode surface used by VaapiH264Decoder to decode into Since this class isn't used in this .h (or .cc file, other than definition) ISTM it should either move out of this header or at least move below VaapiDelegate, which is what this header is named after. https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.h:52: VASurfaceID va_surface_id_; const, if you like https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.h:60: // thread-safe, so we have to perform locking ourselves. Does this mean the public methods of this class may be called on any thread? If so, doco. For example, what are the semantics of one thread calling SubmitBuffer and another calling DestroyPendingBuffers "at the same time"? https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.h:65: // as well as underlying memory for VASurfaces themselves. Does this translate to "none of the other files in this directory should need to include anything from third_party/libva directly"? https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.h:66: class VaapiDelegate : public base::RefCountedThreadSafe<VaapiDelegate> { Is this actually a VaapiWrapper? https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.h:66: class VaapiDelegate : public base::RefCountedThreadSafe<VaapiDelegate> { Why refcount? (in particular, can VAVDA hold a scoped_ptr and the decoder hold a raw pointer and things still be safe?) https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.h:69: // to report errors to clients. Why not rename it to something clearer, then? report_error_to_uma_cb ? https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.h:77: // wrapped in VASurfaces. Returns success. https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.h:78: bool CreateSurfaces(int width, int height, here and elsewhere, better to use a gfx::Size than a pair of ints (whose order is easily confused) https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.h:78: bool CreateSurfaces(int width, int height, The impl of this assumes that this method is called no more than once during the lifetime of a given VaapiDelegate object. The doco should say so. (this will break when you teach the VDAs to deal with midstream resize because you'll want to allow the VDA impls to accept multiple AssignPictureBuffer calls during their lifetime) https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.h:80: std::vector<VASurfaceID>& va_surfaces); Non non-const references. Here and elsewhere, which includes at least: + VAProfile& va_profile) { + std::vector<VASurfaceID>& va_surfaces) { + std::vector<VASurfaceID>& va_surfaces); according to a quickie grep. https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.h:83: void DestroySurfaces(); Should be private. https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.h:98: bool DecodeAndDestroyPendingBuffers(VASurfaceID va_surface_id); Why is "Destroy" part of the name of this function? (IOW, what client-visible state is being destroyed) https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.h:98: bool DecodeAndDestroyPendingBuffers(VASurfaceID va_surface_id); What does the bool return value mean? I can imagine at least: - successfully decoded a pic; more data is available to be decoded - successfully decoded a pic; no more data is available to be decoded - failed to decode a pic b/c of error - failed to decode a pic b/c of insufficient data being submitted ... https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.h:101: // Put data from |va_surface_id| into x_pixmap of size |width|x|height|, |x_pixmap| https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.h:103: bool PutSurfaceIntoPixmap(VASurfaceID va_surface_id, Does this require making a context current (or somesuch) before calling this? https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.h:105: int width, int height); dest_width & dest_height ? https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.h:115: bool Initialize(media::VideoCodecProfile profile, Should be private, and can be inlined into Create(). https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.h:118: void Deinitialize(); inline into dtor, since it's the only callsite and also because it's not possible to re-Initialize() after Deinitialize() runs? https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.h:120: bool SubmitDecode(VASurfaceID va_surface_id); doco methods whose purpose is unclear. For example here, it's unclear why this is not inlined into DecodeAndDestroyPendingBuffers. Also, it's extra unclear because this method is named SubmitDecode, but is not called by SubmitBuffer. https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.h:123: // This lock is to be taken for the duration of all VA-API calls and for I think the only API that makes sense is that every public method of this class acquires the lock for the duration of its execution. A salient point for why this strategy _might_ be correct is that this class is fully synchronous. I think it's worth calling out in the class doco that explains about threading. https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.h:124: // the entire decode execution sequence in DecodeAndDestroyPendingBuffers(). ...and yet, it is not (see comment there). https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.h:125: base::Lock va_lock_; naming this va_lock_ implies it should be held only around libva calls, which is incorrect (you want to hold it across libva calls within the same VaapiDelegate function) https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.h:127: // Allocated backing memory ids for VASurfaces. This comment emphasizes "allocated" in a surprising way to me. https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.h:129: size_t num_va_surfaces_; why not std::vector<VASurfaceID> instead and avoid the need for the explicit num_ ? If the motivation here was that you need a contiguous buffer to feed to VAAPI_ functions, you can always do: va_surface_ids_.resize(num_surfaces_or_whatever); VAAPI_Foo(&va_surface_ids_[0], va_surface_ids_.size()); https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.h:134: VAContextID va_context_id_; Would be useful to annotate these with when in the lifecycle they become valid/invalid. https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.h:138: std::vector<VABufferID> pending_va_bufs_; Do these really need to be separate? (AFAICT they're all fed to the same APIs; does VAAPI require to see non-slices before slices??) https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_delegate.h:146: static bool PostSandboxInitialization(); methods go above members https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... File content/common/gpu/media/vaapi_h264_decoder.cc (right): https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder.cc:10: #include "base/memory/ref_counted.h" how is this not required by the .h? https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder.cc:19: class VaapiH264Decoder::DecodeSurface { Would this be better off as an almost-POD now that it has no behavior of its own? struct VaapiH264Decoder::DecodeSurface { ctor, dtor int poc; int32 input_id; const scoped_refptr<VASurface>& va_surface; }; ? https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder.cc:31: VASurfaceID va_surface_id() { Is this really adding value compared with va_surface()->id() ? https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder.cc:70: curr_input_id_ = -1; does this not belong inside Reset()? https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder.cc:73: pic_size_ = gfx::Size(); noop https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder.cc:78: last_output_poc_ = 0; any reason these initializers couldn't/shouldn't go in the initializer list of the ctor? https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder.cc:86: // a random location in the stream. this is an API comment, not an impl comment? https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder.cc:215: void VaapiH264Decoder::UnassignSurfaceFromPoC(int poc) { IWBN for there to be an ascii (or .dot) diagram + explanation in one of the headers that tells what the lifecycle of surfaces, buffers, textures, and pixmaps look like. In particular it's difficult to see why at this point once the surface has been unassigned from the POC it's not ready to be re-added to available_va_surfaces_. https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder.cc:462: // TODO(posciak) start using vaMapBuffer instead of vaCreateBuffer wherever comment trails off https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder.cc:1005: if (pic->outputted) how can this happen? https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder.cc:1055: ClearDPB(); Why not clear the DPB if outputting fails above? https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... File content/common/gpu/media/vaapi_h264_decoder.h (right): https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder.h:12: #include <list> unused if you take my suggestion below. https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder.h:34: class VaapiH264Decoder { doco threaded characteristics (which I believe are "must be created, called, and destroyed on a single thread, and does nothing internally on any other thread"). https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder.h:42: enum VAVDAH264DecoderFailure { s/VAVDA/VAAPI/ ? https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder.h:53: typedef base::Callback<void(VAVDAH264DecoderFailure err)> ReportErrorCB; s/err/error/ https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder.h:53: typedef base::Callback<void(VAVDAH264DecoderFailure err)> ReportErrorCB; ReportErrorToUmaCB https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder.h:79: // Returns false if any of the resulting invocations of the callback fail. OutputPicCB returns void; how can its invocation "fail"? https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder.h:84: // but do not flush decoder state, so that the playback can be resumed laster, typo: laster https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder.h:100: DecResult DecodeOneFrame(int32 input_id) WARN_UNUSED_RESULT; should |input_id| (here and in DecodeInitial) actually be a param to SetStream()? https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder.h:181: bool ConcealLostIDR(); These three lines fell from the future through a time vortex of some sort? https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder.h:184: // Cleared pictures will be made available for decode, unless they are s/pictures/surfaces/ ? https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder.h:257: DecodeSurface* DecodeSurfaceByPoC(int poc); Here and elsewhere: methods precede members. https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_h264_decoder.h:260: std::list<scoped_refptr<VASurface> > available_va_surfaces_; FWIW could use a vector and always pop from the back, which would be slightly more efficient but more importantly would mean you use vector for everything in this class instead of mixing list & vector. https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_video_decode_accelerator.cc:19: namespace { pointless https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_video_decode_accelerator.cc:24: "Media.VAVDAH264.DecoderFailure", This is clearly a decoder thing, not a VAVDA thing. I think you have it here instead of in the decoder because you want to reuse it for the delegate but it's icky. https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_video_decode_accelerator.cc:79: int32 picture_buffer_id() { Please rationalize order of ctor args, getters, and members. https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_video_decode_accelerator.cc:136: picture_buffer_id_(picture_buffer_id), indent here and below https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_video_decode_accelerator.cc:219: gfx::ScopedTextureBinder texture_binder(GL_TEXTURE_2D, texture_id_); Is it really correct to have this be scoped to just this and the next line? Does the binding not have to last through the put? https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_video_decode_accelerator.cc:574: VASurface::ReleaseCB va_surface_relase_cb = typo: va_surface_relase_cb https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_video_decode_accelerator.cc:603: base::AutoUnlock auto_unlock(lock_); Comment about why and why safe. https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... File content/common/gpu/media/vaapi_video_decode_accelerator.h (right): https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_video_decode_accelerator.h:88: bool GetOutputSurfaces_Locked(); function name is crying out for improvement https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_video_decode_accelerator.h:137: void SyncSurfaceToPicture(VASurfaceID va_surface_id, TFPPicture* tfp_picture); This CL finaly makes it possible for this madness to stop! (this method can now be inlined into OutputPicture). https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_video_decode_accelerator.h:179: State state_; should go under lock_ https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_video_decode_accelerator.h:218: gfx::Size pic_size_; You can drop both of these in favor of simply calling the decoder_ methods twice. https://codereview.chromium.org/14914009/diff/3001/content/common/gpu/media/v... content/common/gpu/media/vaapi_video_decode_accelerator.h:256: scoped_ptr<VaapiH264Decoder> decoder_; this should go above thread_ so that it's easy to see that the decoder can't be deleted before the thread is stopped/joined/deleted.
https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... File content/common/gpu/media/vaapi_delegate.cc (right): https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.cc:11: #define LOG_VA_ERROR_AND_REPORT(va_res, err_msg) \ On 2013/05/17 23:19:15, Ami Fischman wrote: > s/va_res/va_error/ here and in the next 4 lines Well, the result might be a success too, so I'd prefer keeping res, unless you have strong feelings about this. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.cc:35: void *vaapi_x11_handle = NULL; On 2013/05/17 23:19:15, Ami Fischman wrote: > static (this & above) Good point. Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.cc:106: #define VAAPI_SYM(name, handle) Vaapi##name VAAPI_##name = NULL On 2013/05/17 23:19:15, Ami Fischman wrote: > #undef after done w/ these? Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.cc:126: VAAPI_SYM(ErrorStr, vaapi_handle); On 2013/05/17 23:19:15, Ami Fischman wrote: > Any reason not to do these (and the typedefs above and the dlsym's at the bottom > of the file) in sorted order? Well, they are thematically and chronologically sorted. CreateSurfaces() comes together with DestroySurfaces() and they are called before CreateContext() in the actual flow as well. I can apply my dictionary sorting skills if you prefer it that way though. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.cc:130: VAProfile& va_profile) { On 2013/05/17 23:19:15, Ami Fischman wrote: > non-const ref Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.cc:138: case media::H264PROFILE_HIGH: On 2013/05/17 23:19:15, Ami Fischman wrote: > What about the variants of HIGH in media? Do they not get handled by VA's > "high"? (also, does using VA "high" for media's EXTENDED not work?) The other ones are "HIGH10", 422 and 444, I might've had a reason for this, but I don't remember now. I'll add a TODO to look at it again. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.cc:173: scoped_refptr<VaapiDelegate> vaapi_delegate(new VaapiDelegate); On 2013/05/17 23:19:15, Ami Fischman wrote: > nit: never omit the parens on a new statement. Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.cc:184: static bool vaapi_functions_initialized = PostSandboxInitialization(); On 2013/05/17 23:19:15, Ami Fischman wrote: > This idiom is not thread-safe. > (https://groups.google.com/a/chromium.org/forum/#%21msg/chromium-dev/p6h3HC8Wr...) Yes, but only one thread ever runs this... https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.cc:212: VAConfigAttrib attrib; On 2013/05/17 23:19:15, Ami Fischman wrote: > s/;/ = {0};/ ? Done (slightly differently). https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.cc:215: VAEntrypoint entrypoint = VAEntrypointVLD; On 2013/05/17 23:19:15, Ami Fischman wrote: > necessary? > (at the very least, should be const & name begin with k) Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.cc:273: DestroySurfaces(); On 2013/05/17 23:19:15, Ami Fischman wrote: > I think this isn't really necessary, in which case inline DestroySurfaces into > the dtor. Please see .h about inlining DestroySurfaces(). I'd prefer keeping it this way, doesn't really make a big difference, but at least releases us from having a contract that you can't do anything with the class after this fails. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.cc:278: va_surfaces.push_back(va_surface_ids_[i]); On 2013/05/17 23:19:15, Ami Fischman wrote: > Once the member is a vector too you can simply assign the vector here (not > member-wise). Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.cc:351: std::copy(pending_va_bufs_.begin(), pending_va_bufs_.end(), va_buffers.get()); On 2013/05/17 23:19:15, Ami Fischman wrote: > Why the copies? Is VAAPI_RenderPicture destructive of its arguments? > Or can you instead simply use &pending_va_bufs_[0] for the param? Yes, getting over my distaste for this syntax... https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.cc:356: slice_buffers.get()); On 2013/05/17 23:19:15, Ami Fischman wrote: > ditto Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.cc:381: bool result = SubmitDecode(va_surface_id); On 2013/05/17 23:19:15, Ami Fischman wrote: > The lock is not held across this & the next line, meaning that one thread's > DPB() call may nuke another thread's just-submitted DADPB() call? > > Put another way, I suspect a better layout would be to rename > DestroyPendingBuffers to DestroyPendingBuffers_Locked, make > DestroyPendingBuffers acquire the lock and call DestroyPendingBuffers_Locked(), > inline SubmitDecode into DADPB(), and call DestroyPendingBuffers_Locked() from > the bottom of DADPB. I actually had it that way initially. But it was too much unnecessary code, because then I needed a non-Locked() version of DestroyPendingBuffers() for destroying buffers without Decode()ing and a _Locked() one for destroying after decoding. But there is no problem here. First of all, using this class in such a way from multiple threads is not really allowed, Destroy could be called at any point before a locked Decode to mess up things anyway. It's locked well enough for the Decode (and HW itself) not to explode, because the whole Decode is locked. If something calls Destroy, it will execute only before or after a full Decode. Before, it will make Decode() fail, but it's not really like we could prevent this, because it could have been called before we locked ourselves in DecodeAndDestroyPendingBuffers anyway. And if the other thread happens to Destroy() after Decode(), but before Destroy() executes here, the Destroy() here will be a noop, but everything will stay in order, again not exploding. What you are trying to prevent is not preventable and not really a scenario that is supported to work, but what is now here is sufficient to prevent things from exploding at least. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.cc:399: 0, 0, width, height, On 2013/05/17 23:19:15, Ami Fischman wrote: > is there an assumption that this class only knows about coded sizes, but not > visible rect or padding issues? Correct. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.cc:445: return VAAPI_GetDisplay && On 2013/05/17 23:19:15, Ami Fischman wrote: > FWIW you could test for NULL and early-return as part of the macro, which would > also let you emit a more useful log message (about which symbol failed to load). Yeah, but honestly I don't think it'd really be that useful... It's just two specific .so's that we have for all those functions. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... File content/common/gpu/media/vaapi_delegate.h (right): https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:21: // A VA-API-specific decode surface used by VaapiH264Decoder to decode into On 2013/05/17 23:19:15, Ami Fischman wrote: > Since this class isn't used in this .h (or .cc file, other than definition) ISTM > it should either move out of this header or at least move below VaapiDelegate, > which is what this header is named after. Didn't want to create a separate header for it, I'll move below. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:52: VASurfaceID va_surface_id_; On 2013/05/17 23:19:15, Ami Fischman wrote: > const, if you like Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:60: // thread-safe, so we have to perform locking ourselves. On 2013/05/17 23:19:15, Ami Fischman wrote: > Does this mean the public methods of this class may be called on any thread? If > so, doco. > For example, what are the semantics of one thread calling SubmitBuffer and > another calling DestroyPendingBuffers "at the same time"? It's safe to call from any thread, things will not explode. But some obvious sanity is required for things to work; even if nothing will explode if the above situation happens, if one thread blows up buffers for another, the decode will fail and return an error. But no leaks or use after free will happen, as Decode() operations are atomic, both in this class and in the driver. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:65: // as well as underlying memory for VASurfaces themselves. On 2013/05/17 23:19:15, Ami Fischman wrote: > Does this translate to "none of the other files in this directory should need to > include anything from third_party/libva directly"? Correct. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:66: class VaapiDelegate : public base::RefCountedThreadSafe<VaapiDelegate> { On 2013/05/17 23:19:15, Ami Fischman wrote: > Why refcount? > (in particular, can VAVDA hold a scoped_ptr and the decoder hold a raw pointer > and things still be safe?) Currently yes, as the decoder doesn't do anything with the delegate in its destructor. I don't expect it to start doing that, but if it were to, we would have to ensure the decoder was destroyed before the delegate. So is what you describe preferred? This was just defensive programming, what's the usual thing we do in this situation? https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:66: class VaapiDelegate : public base::RefCountedThreadSafe<VaapiDelegate> { On 2013/05/17 23:19:15, Ami Fischman wrote: > Is this actually a VaapiWrapper? For me delegate and wrapper are almost synonymous in this case, but I can call it a wrapper if you prefer. Do you? https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:69: // to report errors to clients. On 2013/05/17 23:19:15, Ami Fischman wrote: > Why not rename it to something clearer, then? > report_error_to_uma_cb ? Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:77: // wrapped in VASurfaces. On 2013/05/17 23:19:15, Ami Fischman wrote: > Returns success. Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:78: bool CreateSurfaces(int width, int height, On 2013/05/17 23:19:15, Ami Fischman wrote: > here and elsewhere, better to use a gfx::Size than a pair of ints (whose order > is easily confused) Done, here and in other places. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:78: bool CreateSurfaces(int width, int height, On 2013/05/17 23:19:15, Ami Fischman wrote: > The impl of this assumes that this method is called no more than once during the > lifetime of a given VaapiDelegate object. The doco should say so. > Not exactly. CreateSurfaces() -> DestroySurfaces() -> CreateSurfaces() is a perfectly valid sequence. > (this will break when you teach the VDAs to deal with midstream resize because > you'll want to allow the VDA impls to accept multiple AssignPictureBuffer calls > during their lifetime) Well, that's why DestroySurfaces() is not private. Doing DestroySurfaces() from Create - if someone called it again - is not safe either, because clients may still be using them. So I only have a DCHECK in Create for this. I could have them refcounted, but chose not to. First of all, I'd need a second refcounted wrapper around IDs, and I have one already (VASurface, but it's not for destroying the IDs, just for putting VASurfaces back into a freelist), which feels a bit like overdoing this. And second of all, each such surface would also have to keep a ref to the delegate. So it's a bit meh... Are you ok with just the DCHECK and a contract in documentation? https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:80: std::vector<VASurfaceID>& va_surfaces); On 2013/05/17 23:19:15, Ami Fischman wrote: > Non non-const references. > Here and elsewhere, which includes at least: > + VAProfile& va_profile) { > + std::vector<VASurfaceID>& va_surfaces) { > + std::vector<VASurfaceID>& va_surfaces); > > according to a quickie grep. Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:83: void DestroySurfaces(); On 2013/05/17 23:19:15, Ami Fischman wrote: > Should be private. See above (l.78). https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:98: bool DecodeAndDestroyPendingBuffers(VASurfaceID va_surface_id); On 2013/05/17 23:19:15, Ami Fischman wrote: > Why is "Destroy" part of the name of this function? > (IOW, what client-visible state is being destroyed) I felt documentation for SubmitBuffer explained it well enough: // Data submitted via this method awaits in the HW decoder until // DecodeAndDestroyPendingBuffers is called to execute or // DestroyPendingBuffers is used to cancel a pending decode Destroy means "dispose of the buffers I submitted to the decoder via SubmitBuffer". https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:98: bool DecodeAndDestroyPendingBuffers(VASurfaceID va_surface_id); On 2013/05/17 23:19:15, Ami Fischman wrote: > What does the bool return value mean? > I can imagine at least: > - successfully decoded a pic; more data is available to be decoded > - successfully decoded a pic; no more data is available to be decoded > - failed to decode a pic b/c of error > - failed to decode a pic b/c of insufficient data being submitted > ... Decode() is for exactly one picture in vaapi. So the caller must've submitted correct and sufficient data to decode exactly one picture. Otherwise the decode will fail, and it's not really intended to be recoverable right now. So 1) and 4) cannot happen. I could've elaborated on this, but let's face it, this is not a reusable class that anybody can just start using. The client is tightly tied to this class (or perhaps the other way around) and has to know exactly how to use vaapi in all the other places, where it constructs VABuffers, submits them, etc. So I didn't want to overdocument, it's really not a very "public" interface that just about anyone could start using. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:101: // Put data from |va_surface_id| into x_pixmap of size |width|x|height|, On 2013/05/17 23:19:15, Ami Fischman wrote: > |x_pixmap| Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:103: bool PutSurfaceIntoPixmap(VASurfaceID va_surface_id, On 2013/05/17 23:19:15, Ami Fischman wrote: > Does this require making a context current (or somesuch) before calling this? No, thankfully. It's not a GL call. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:105: int width, int height); On 2013/05/17 23:19:15, Ami Fischman wrote: > dest_width & dest_height ? dest_size :) https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:115: bool Initialize(media::VideoCodecProfile profile, On 2013/05/17 23:19:15, Ami Fischman wrote: > Should be private, and can be inlined into Create(). It is private :) I didn't want to inline, because I'd have to have a '= NULL' and return every time something failed. This way I have them once. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:118: void Deinitialize(); On 2013/05/17 23:19:15, Ami Fischman wrote: > inline into dtor, since it's the only callsite and also because it's not > possible to re-Initialize() after Deinitialize() runs? I'd prefer to keep it this way, makes the destructor cleaner and code more understandable with Initialize-Deinitialize pair. I really wanted to keep it tidy and clear. And yes, it's ok to re-Initialize() after Deinitialize(), even if nothing does that now. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:120: bool SubmitDecode(VASurfaceID va_surface_id); On 2013/05/17 23:19:15, Ami Fischman wrote: > doco methods whose purpose is unclear. For example here, it's unclear why this > is not inlined into DecodeAndDestroyPendingBuffers. > > Also, it's extra unclear because this method is named SubmitDecode, but is not > called by SubmitBuffer. To be honest I don't feel that way: SubmitBuffer(); SubmitBuffer(); SubmitBuffer(); ... SubmitDecode(); feels like a natural sequence to me. I don't see why SubmitDecode should be called by SubmitBuffer... But since you thought of it like that, it's my failure, not documenting it well enough... https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:123: // This lock is to be taken for the duration of all VA-API calls and for On 2013/05/17 23:19:15, Ami Fischman wrote: > I think the only API that makes sense is that every public method of this class > acquires the lock for the duration of its execution. > > A salient point for why this strategy _might_ be correct is that this class is > fully synchronous. I think it's worth calling out in the class doco that > explains about threading. Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:124: // the entire decode execution sequence in DecodeAndDestroyPendingBuffers(). On 2013/05/17 23:19:15, Ami Fischman wrote: > ...and yet, it is not (see comment there). See comment there please. Destroy() doesn't have to be in the same critical section as Decode, just cannot come in the middle of it, which it doesn't. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:125: base::Lock va_lock_; On 2013/05/17 23:19:15, Ami Fischman wrote: > naming this va_lock_ implies it should be held only around libva calls, which is > incorrect (you want to hold it across libva calls within the same VaapiDelegate > function) This is a lock around each libva call or calls that need to be called atomically in a sequence. There is only one example of the latter, in Decode(). https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:127: // Allocated backing memory ids for VASurfaces. On 2013/05/17 23:19:15, Ami Fischman wrote: > This comment emphasizes "allocated" in a surprising way to me. Well, this was also kind-of documenting what the ids were. Anyway, removing. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:129: size_t num_va_surfaces_; On 2013/05/17 23:19:15, Ami Fischman wrote: > why not std::vector<VASurfaceID> instead and avoid the need for the explicit > num_ ? > > If the motivation here was that you need a contiguous buffer to feed to VAAPI_ > functions, you can always do: > va_surface_ids_.resize(num_surfaces_or_whatever); > VAAPI_Foo(&va_surface_ids_[0], va_surface_ids_.size()); Yeah, I know this full well, and I even thought about it, but maybe it's the C programmer in me that I have this weird aversion and mistrust to ever doing this... Maybe it's a good time to get over it. Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:134: VAContextID va_context_id_; On 2013/05/17 23:19:15, Ami Fischman wrote: > Would be useful to annotate these with when in the lifecycle they become > valid/invalid. Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:138: std::vector<VABufferID> pending_va_bufs_; On 2013/05/17 23:19:15, Ami Fischman wrote: > Do these really need to be separate? > (AFAICT they're all fed to the same APIs; does VAAPI require to see non-slices > before slices??) All the code that used VAAPI that I've seen, including the tests and code written by Intel VAAPI developers does the same thing. Even if it was possible now, who knows when this might break. I'd prefer not to go against the flow here, as much as I don't like this separation myself either. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:146: static bool PostSandboxInitialization(); On 2013/05/17 23:19:15, Ami Fischman wrote: > methods go above members Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... File content/common/gpu/media/vaapi_h264_decoder.cc (right): https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.cc:10: #include "base/memory/ref_counted.h" On 2013/05/17 23:19:15, Ami Fischman wrote: > how is this not required by the .h? vaapi_delegate.h included from there includes it. But the right way to do this is to move this line to decoder's .h anyway. Doing so. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.cc:19: class VaapiH264Decoder::DecodeSurface { On 2013/05/17 23:19:15, Ami Fischman wrote: > Would this be better off as an almost-POD now that it has no behavior of its > own? > > struct VaapiH264Decoder::DecodeSurface { > ctor, dtor > int poc; > int32 input_id; > const scoped_refptr<VASurface>& va_surface; > }; > ? Well, taking a quick peek into the vortex, there's some more stuff falling back into this class pretty soon, which includes another constructor. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.cc:31: VASurfaceID va_surface_id() { On 2013/05/17 23:19:15, Ami Fischman wrote: > Is this really adding value compared with va_surface()->id() ? Avoid a second call? I bet you'll say this is not a reason enough... https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.cc:70: curr_input_id_ = -1; On 2013/05/17 23:19:15, Ami Fischman wrote: > does this not belong inside Reset()? Nice catch, thanks. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.cc:73: pic_size_ = gfx::Size(); On 2013/05/17 23:19:15, Ami Fischman wrote: > noop Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.cc:78: last_output_poc_ = 0; On 2013/05/17 23:19:15, Ami Fischman wrote: > any reason these initializers couldn't/shouldn't go in the initializer list of > the ctor? Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.cc:86: // a random location in the stream. On 2013/05/17 23:19:15, Ami Fischman wrote: > this is an API comment, not an impl comment? Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.cc:215: void VaapiH264Decoder::UnassignSurfaceFromPoC(int poc) { On 2013/05/17 23:19:15, Ami Fischman wrote: > IWBN for there to be an ascii (or .dot) diagram + explanation in one of the > headers that tells what the lifecycle of surfaces, buffers, textures, and > pixmaps look like. > In particular it's difficult to see why at this point once the surface has been > unassigned from the POC it's not ready to be re-added to available_va_surfaces_. Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.cc:462: // TODO(posciak) start using vaMapBuffer instead of vaCreateBuffer wherever On 2013/05/17 23:19:15, Ami Fischman wrote: > comment trails off Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.cc:1005: if (pic->outputted) On 2013/05/17 23:19:15, Ami Fischman wrote: > how can this happen? Another visitor from the future, but a harmless one. Let's keep it. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.cc:1055: ClearDPB(); On 2013/05/17 23:19:15, Ami Fischman wrote: > Why not clear the DPB if outputting fails above? No false return from anywhere will ever result in a recovery attempt, so it'll take care of itself anyway. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... File content/common/gpu/media/vaapi_h264_decoder.h (right): https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.h:12: #include <list> On 2013/05/17 23:19:15, Ami Fischman wrote: > unused if you take my suggestion below. Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.h:34: class VaapiH264Decoder { On 2013/05/17 23:19:15, Ami Fischman wrote: > doco threaded characteristics (which I believe are "must be created, called, and > destroyed on a single thread, and does nothing internally on any other thread"). Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.h:42: enum VAVDAH264DecoderFailure { On 2013/05/17 23:19:15, Ami Fischman wrote: > s/VAVDA/VAAPI/ ? All errors but VAAPI_ERROR are actually VAVDA-specific. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.h:53: typedef base::Callback<void(VAVDAH264DecoderFailure err)> ReportErrorCB; On 2013/05/17 23:19:15, Ami Fischman wrote: > s/err/error/ Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.h:53: typedef base::Callback<void(VAVDAH264DecoderFailure err)> ReportErrorCB; On 2013/05/17 23:19:15, Ami Fischman wrote: > ReportErrorToUmaCB Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.h:79: // Returns false if any of the resulting invocations of the callback fail. On 2013/05/17 23:19:15, Ami Fischman wrote: > OutputPicCB returns void; how can its invocation "fail"? Yeah, fallout. Should be if other stuff fails now. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.h:84: // but do not flush decoder state, so that the playback can be resumed laster, On 2013/05/17 23:19:15, Ami Fischman wrote: > typo: laster Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.h:100: DecResult DecodeOneFrame(int32 input_id) WARN_UNUSED_RESULT; On 2013/05/17 23:19:15, Ami Fischman wrote: > should |input_id| (here and in DecodeInitial) actually be a param to > SetStream()? This assumes knowledge of VDA specifics that it splits stream into chunks that produce at most one frame. I don't want to have this constraint here. You should be able to SetStream() for more than one frame, and call DecodeOneFrame() with the input_id you want for the frame that comes next in that stream buffer. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.h:181: bool ConcealLostIDR(); On 2013/05/17 23:19:15, Ami Fischman wrote: > These three lines fell from the future through a time vortex of some sort? Wow, indeed. Final rebase artifact https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.h:184: // Cleared pictures will be made available for decode, unless they are On 2013/05/17 23:19:15, Ami Fischman wrote: > s/pictures/surfaces/ ? Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.h:257: DecodeSurface* DecodeSurfaceByPoC(int poc); On 2013/05/17 23:19:15, Ami Fischman wrote: > Here and elsewhere: methods precede members. Not a big fan of this rule, I prefer things in logical groups... Oh well. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.h:260: std::list<scoped_refptr<VASurface> > available_va_surfaces_; On 2013/05/17 23:19:15, Ami Fischman wrote: > FWIW could use a vector and always pop from the back, which would be slightly > more efficient but more importantly would mean you use vector for everything in > this class instead of mixing list & vector. Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_video_decode_accelerator.cc:19: namespace { On 2013/05/17 23:19:15, Ami Fischman wrote: > pointless Even if at some point a global ReportToUMA() surfaces? https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_video_decode_accelerator.cc:24: "Media.VAVDAH264.DecoderFailure", On 2013/05/17 23:19:15, Ami Fischman wrote: > This is clearly a decoder thing, not a VAVDA thing. > I think you have it here instead of in the decoder because you want to reuse it > for the delegate but it's icky. Yeah, I hate it too, but didn't see a better way to do this... https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_video_decode_accelerator.cc:79: int32 picture_buffer_id() { On 2013/05/17 23:19:15, Ami Fischman wrote: > Please rationalize order of ctor args, getters, and members. Not really anything in particular, but getters are the same order as members. Members are arranged with ids and size first, because they are used outside, then pixmap together with glx pixmap and x_display (just moved) last, because it's used internally. I'm not sure if you have anything in particular in mind? https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_video_decode_accelerator.cc:136: picture_buffer_id_(picture_buffer_id), On 2013/05/17 23:19:15, Ami Fischman wrote: > indent here and below Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_video_decode_accelerator.cc:219: gfx::ScopedTextureBinder texture_binder(GL_TEXTURE_2D, texture_id_); On 2013/05/17 23:19:15, Ami Fischman wrote: > Is it really correct to have this be scoped to just this and the next line? > Does the binding not have to last through the put? No. This saves the texture that was bound (current) when we got here and makes texture_id_ current (glBindTexture it), after this returns our texture_id is current (bound) and it will be used when we call glXBindTexture(). Once we return from this function the old texture is restored by the Binder. But this is orthogonal to binding to texture, which stays bound to pixmap. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_video_decode_accelerator.cc:574: VASurface::ReleaseCB va_surface_relase_cb = On 2013/05/17 23:19:15, Ami Fischman wrote: > typo: va_surface_relase_cb Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_video_decode_accelerator.cc:603: base::AutoUnlock auto_unlock(lock_); On 2013/05/17 23:19:15, Ami Fischman wrote: > Comment about why and why safe. Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... File content/common/gpu/media/vaapi_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_video_decode_accelerator.h:88: bool GetOutputSurfaces_Locked(); On 2013/05/17 23:19:15, Ami Fischman wrote: > function name is crying out for improvement Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_video_decode_accelerator.h:137: void SyncSurfaceToPicture(VASurfaceID va_surface_id, TFPPicture* tfp_picture); On 2013/05/17 23:19:15, Ami Fischman wrote: > This CL finaly makes it possible for this madness to stop! > (this method can now be inlined into OutputPicture). Yes! Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_video_decode_accelerator.h:179: State state_; On 2013/05/17 23:19:15, Ami Fischman wrote: > should go under lock_ Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_video_decode_accelerator.h:218: gfx::Size pic_size_; On 2013/05/17 23:19:15, Ami Fischman wrote: > You can drop both of these in favor of simply calling the decoder_ methods > twice. This would result in calling the decoder from ChildThread in AssignPictureBuffers, which I didn't want to do. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_video_decode_accelerator.h:256: scoped_ptr<VaapiH264Decoder> decoder_; On 2013/05/17 23:19:15, Ami Fischman wrote: > this should go above thread_ so that it's easy to see that the decoder can't be > deleted before the thread is stopped/joined/deleted. Done.
https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... File content/common/gpu/media/h264_dpb.h (right): https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/h264_dpb.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. WARNING: due to the vagaries of rietveld some of my comments are on the patchset#1 version of files, some are on the patchset #2 version of files. I recommend you either use the email interface to make sure you see all comments, or else view the PS#1->PS#2 diff view in rietveld. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... File content/common/gpu/media/vaapi_delegate.cc (right): https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.cc:11: #define LOG_VA_ERROR_AND_REPORT(va_res, err_msg) \ On 2013/05/21 22:32:35, posciak wrote: > On 2013/05/17 23:19:15, Ami Fischman wrote: > > s/va_res/va_error/ here and in the next 4 lines > > Well, the result might be a success too, so I'd prefer keeping res, unless you > have strong feelings about this. It cannot be success in this macro, that's my point. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.cc:184: static bool vaapi_functions_initialized = PostSandboxInitialization(); On 2013/05/21 22:32:35, posciak wrote: > On 2013/05/17 23:19:15, Ami Fischman wrote: > > This idiom is not thread-safe. > > > (https://groups.google.com/a/chromium.org/forum/#%2521msg/chromium-dev/p6h3HC8...) > > Yes, but only one thread ever runs this... How's that? https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.cc:273: DestroySurfaces(); On 2013/05/21 22:32:35, posciak wrote: > On 2013/05/17 23:19:15, Ami Fischman wrote: > > I think this isn't really necessary, in which case inline DestroySurfaces into > > the dtor. > > Please see .h about inlining DestroySurfaces(). > I'd prefer keeping it this way, doesn't really make a big difference, but at > least releases us from having a contract that you can't do anything with the > class after this fails. Why would you not want to have that contract? https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.cc:445: return VAAPI_GetDisplay && On 2013/05/21 22:32:35, posciak wrote: > On 2013/05/17 23:19:15, Ami Fischman wrote: > > FWIW you could test for NULL and early-return as part of the macro, which > would > > also let you emit a more useful log message (about which symbol failed to > load). > > Yeah, but honestly I don't think it'd really be that useful... It's just two > specific .so's that we have for all those functions. a) it would too be useful (I've had to add that code in a debugging session once before) b) it would allow you to drop this yet-another-list of these symbols (the return statement would simply be "return true;" b/c all the return false's would be part of the macro) Reason b) is actually a good reason to do it this way IMO. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... File content/common/gpu/media/vaapi_delegate.h (right): https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:21: // A VA-API-specific decode surface used by VaapiH264Decoder to decode into On 2013/05/21 22:32:35, posciak wrote: > On 2013/05/17 23:19:15, Ami Fischman wrote: > > Since this class isn't used in this .h (or .cc file, other than definition) > ISTM > > it should either move out of this header or at least move below VaapiDelegate, > > which is what this header is named after. > > Didn't want to create a separate header for it, I'll move below. I think this is a bad tradeoff. It's a separate thing with separate needs and is unused by the class this file is named after so IMO it should have its own header. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:66: class VaapiDelegate : public base::RefCountedThreadSafe<VaapiDelegate> { On 2013/05/21 22:32:35, posciak wrote: > On 2013/05/17 23:19:15, Ami Fischman wrote: > > Why refcount? > > (in particular, can VAVDA hold a scoped_ptr and the decoder hold a raw pointer > > and things still be safe?) > > Currently yes, as the decoder doesn't do anything with the delegate in its > destructor. I don't expect it to start doing that, but if it were to, we would > have to ensure the decoder was destroyed before the delegate. So is what you > describe preferred? This was just defensive programming, what's the usual thing > we do in this situation? Refcounting abdicates responsibility for ownership management. It is "more defensive" than managing ownership explicitly, but only insofar as leaking all allocations is "more defensive" than cleaning up unused garbage ;) If refcounting is not needed please manage ownership explicitly (via scoped_ptr). https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:138: std::vector<VABufferID> pending_va_bufs_; On 2013/05/21 22:32:35, posciak wrote: > On 2013/05/17 23:19:15, Ami Fischman wrote: > > Do these really need to be separate? > > (AFAICT they're all fed to the same APIs; does VAAPI require to see non-slices > > before slices??) > > All the code that used VAAPI that I've seen, including the tests and code > written by Intel VAAPI developers does the same thing. Even if it was possible > now, who knows when this might break. I'd prefer not to go against the flow > here, as much as I don't like this separation myself either. You just gave a great definition of what it means to cargo-cult. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... File content/common/gpu/media/vaapi_h264_decoder.cc (right): https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.cc:31: VASurfaceID va_surface_id() { On 2013/05/21 22:32:35, posciak wrote: > On 2013/05/17 23:19:15, Ami Fischman wrote: > > Is this really adding value compared with va_surface()->id() ? > > Avoid a second call? I bet you'll say this is not a reason enough... lol https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.cc:1005: if (pic->outputted) On 2013/05/21 22:32:35, posciak wrote: > On 2013/05/17 23:19:15, Ami Fischman wrote: > > how can this happen? > > Another visitor from the future, but a harmless one. Let's keep it. That is a terrible logic. Head-scratchers should always have a comment if they can't be deleted. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... File content/common/gpu/media/vaapi_h264_decoder.h (right): https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.h:42: enum VAVDAH264DecoderFailure { On 2013/05/21 22:32:35, posciak wrote: > On 2013/05/17 23:19:15, Ami Fischman wrote: > > s/VAVDA/VAAPI/ ? > > All errors but VAAPI_ERROR are actually VAVDA-specific. My point was that VaapiH264Decoder is a thing, and VaapiVDA is a thing, but this enum name combines "VAVDA" and "H264Decoder", which are not a thing. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.h:100: DecResult DecodeOneFrame(int32 input_id) WARN_UNUSED_RESULT; On 2013/05/21 22:32:35, posciak wrote: > On 2013/05/17 23:19:15, Ami Fischman wrote: > > should |input_id| (here and in DecodeInitial) actually be a param to > > SetStream()? > > This assumes knowledge of VDA specifics that it splits stream into chunks that > produce at most one frame. I don't see that, plus it's not true (a single NALU can complete multiple frames that were coded relative to it). > I don't want to have this constraint here. You should be able to SetStream() for > more than one frame, and call DecodeOneFrame() with the input_id you want for > the frame that comes next in that stream buffer. On the contrary, I believe that this API implies that the decoder's client knows where frame boundaries live. In reality input_id is 1:1 to only input buffers, which are the inputs to SetStream(), so ISTM that SetStream is the place where input_ids should be specified. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_video_decode_accelerator.cc:19: namespace { On 2013/05/21 22:32:35, posciak wrote: > On 2013/05/17 23:19:15, Ami Fischman wrote: > > pointless > > Even if at some point a global ReportToUMA() surfaces? Anything global in the future should be namespaced, so yes. https://chromiumcodereview.appspot.com/14914009/diff/12001/content/common/gpu... File content/common/gpu/media/vaapi_delegate.cc (right): https://chromiumcodereview.appspot.com/14914009/diff/12001/content/common/gpu... content/common/gpu/media/vaapi_delegate.cc:125: VAAPI_SYM(DestroyBuffer, vaapi_handle); Please lexicographically sort the 3 lists involved w/ these sym's. https://chromiumcodereview.appspot.com/14914009/diff/12001/content/common/gpu... File content/common/gpu/media/vaapi_delegate.h (right): https://chromiumcodereview.appspot.com/14914009/diff/12001/content/common/gpu... content/common/gpu/media/vaapi_delegate.h:32: class VaapiDelegate : public base::RefCountedThreadSafe<VaapiDelegate> { please rename to wrapper. Delegate means a different thing to wrapper (if anything, FooDelegate would be the thing that a FooWrapper wraps, as it is the thing that is dispatched to, not the thing doing the dispatching). https://chromiumcodereview.appspot.com/14914009/diff/12001/content/common/gpu... content/common/gpu/media/vaapi_delegate.h:34: // |report_error_to_uma_cb| will be used to report errors for UMA purposes, nit: with the gloriously informative new name, this comment is less necessary. https://chromiumcodereview.appspot.com/14914009/diff/12001/content/common/gpu... content/common/gpu/media/vaapi_delegate.h:67: // Execute decode in hardware and destroy pending buffers. what does a false return value mean? more data necessary? stream error? etc. https://chromiumcodereview.appspot.com/14914009/diff/12001/content/common/gpu... content/common/gpu/media/vaapi_delegate.h:75: gfx::Size dest_size); Is it ever the case that the pixmap & the surface have different sizes? https://chromiumcodereview.appspot.com/14914009/diff/12001/content/common/gpu... content/common/gpu/media/vaapi_delegate.h:90: bool SubmitDecode(VASurfaceID va_surface_id); ditto q about meaning of return false. https://chromiumcodereview.appspot.com/14914009/diff/12001/content/common/gpu... content/common/gpu/media/vaapi_delegate.h:125: nit: add an extra \n I think. https://chromiumcodereview.appspot.com/14914009/diff/12001/content/common/gpu... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/14914009/diff/12001/content/common/gpu... content/common/gpu/media/vaapi_video_decode_accelerator.cc:80: const GLXFBConfig& fb_config, I was asking for the members, accessors, params to Create, and params to ctor be in the same order. I don't really care which order, as long as they all match. https://chromiumcodereview.appspot.com/14914009/diff/12001/content/content_co... File content/content_common.gypi (right): https://chromiumcodereview.appspot.com/14914009/diff/12001/content/content_co... content/content_common.gypi:453: '<(DEPTH)/third_party/khronos', Argh please don't rebase & address comments in the same upload! Address comments first, upload, then rebase & upload new patchset. https://chromiumcodereview.appspot.com/14914009/diff/12001/media/filters/gpu_... File media/filters/gpu_video_decoder.cc (right): https://chromiumcodereview.appspot.com/14914009/diff/12001/media/filters/gpu_... media/filters/gpu_video_decoder.cc:378: //DCHECK_EQ(status != DemuxerStream::kOk, !buffer) << status; lolwat? Planning to revert this file?
https://codereview.chromium.org/14914009/diff/12001/content/common/gpu/media/... File content/common/gpu/media/vaapi_delegate.cc (right): https://codereview.chromium.org/14914009/diff/12001/content/common/gpu/media/... content/common/gpu/media/vaapi_delegate.cc:224: DVLOG(1) << "YUV420 not supported"; nit: that message is a bit hard to grasp at first, could it be something like "The vaapi implementation doesn't support YUV420"? https://codereview.chromium.org/14914009/diff/12001/content/common/gpu/media/... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://codereview.chromium.org/14914009/diff/12001/content/common/gpu/media/... content/common/gpu/media/vaapi_video_decode_accelerator.cc:215: DCHECK(glx_pixmap_); Maybe here CHECK/DCHECK that you're on the main thread? And probably in the other functions which use X/glX as well.
https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... File content/common/gpu/media/vaapi_delegate.cc (right): https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.cc:11: #define LOG_VA_ERROR_AND_REPORT(va_res, err_msg) \ On 2013/05/22 23:59:47, Ami Fischman wrote: > On 2013/05/21 22:32:35, posciak wrote: > > On 2013/05/17 23:19:15, Ami Fischman wrote: > > > s/va_res/va_error/ here and in the next 4 lines > > > > Well, the result might be a success too, so I'd prefer keeping res, unless you > > have strong feelings about this. > > It cannot be success in this macro, that's my point. Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.cc:184: static bool vaapi_functions_initialized = PostSandboxInitialization(); On 2013/05/22 23:59:47, Ami Fischman wrote: > On 2013/05/21 22:32:35, posciak wrote: > > On 2013/05/17 23:19:15, Ami Fischman wrote: > > > This idiom is not thread-safe. > > > > > > (https://groups.google.com/a/chromium.org/forum/#%252521msg/chromium-dev/p6h3H...) > > > > Yes, but only one thread ever runs this... > > How's that? I meant PreSandboxInitialization is run once, which would otherwise cause some problems, but Post yeah, can be run more than once, although it's not really a leak of any sort and shouldn't have other ill effects, since dlsyms are just returning addresses and in practice they will work the same way if executed more than once. And well, as long as assignments are atomic on the platform. Should be fixed nevertheless, submitted a bug, same thing happens in EVDA: crbug.com/243597 https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.cc:273: DestroySurfaces(); On 2013/05/22 23:59:47, Ami Fischman wrote: > On 2013/05/21 22:32:35, posciak wrote: > > On 2013/05/17 23:19:15, Ami Fischman wrote: > > > I think this isn't really necessary, in which case inline DestroySurfaces > into > > > the dtor. > > > > Please see .h about inlining DestroySurfaces(). > > I'd prefer keeping it this way, doesn't really make a big difference, but at > > least releases us from having a contract that you can't do anything with the > > class after this fails. > > Why would you not want to have that contract? I prefer not having contracts when they can be avoided, even if they are very unlikely (well, that is actually another argument for not having the contract). I'm not sure what are we disagreeing here about, is it to save us this one line of code? https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.cc:445: return VAAPI_GetDisplay && On 2013/05/22 23:59:47, Ami Fischman wrote: > On 2013/05/21 22:32:35, posciak wrote: > > On 2013/05/17 23:19:15, Ami Fischman wrote: > > > FWIW you could test for NULL and early-return as part of the macro, which > > would > > > also let you emit a more useful log message (about which symbol failed to > > load). > > > > Yeah, but honestly I don't think it'd really be that useful... It's just two > > specific .so's that we have for all those functions. > > a) it would too be useful (I've had to add that code in a debugging session once > before) > b) it would allow you to drop this yet-another-list of these symbols (the return > statement would simply be "return true;" b/c all the return false's would be > part of the macro) > > Reason b) is actually a good reason to do it this way IMO. Yeah, I see what you meant now, I like it. Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... File content/common/gpu/media/vaapi_delegate.h (right): https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:21: // A VA-API-specific decode surface used by VaapiH264Decoder to decode into On 2013/05/22 23:59:47, Ami Fischman wrote: > On 2013/05/21 22:32:35, posciak wrote: > > On 2013/05/17 23:19:15, Ami Fischman wrote: > > > Since this class isn't used in this .h (or .cc file, other than definition) > > ISTM > > > it should either move out of this header or at least move below > VaapiDelegate, > > > which is what this header is named after. > > > > Didn't want to create a separate header for it, I'll move below. > > I think this is a bad tradeoff. It's a separate thing with separate needs and > is unused by the class this file is named after so IMO it should have its own > header. Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:66: class VaapiDelegate : public base::RefCountedThreadSafe<VaapiDelegate> { On 2013/05/22 23:59:47, Ami Fischman wrote: > On 2013/05/21 22:32:35, posciak wrote: > > On 2013/05/17 23:19:15, Ami Fischman wrote: > > > Why refcount? > > > (in particular, can VAVDA hold a scoped_ptr and the decoder hold a raw > pointer > > > and things still be safe?) > > > > Currently yes, as the decoder doesn't do anything with the delegate in its > > destructor. I don't expect it to start doing that, but if it were to, we would > > have to ensure the decoder was destroyed before the delegate. So is what you > > describe preferred? This was just defensive programming, what's the usual > thing > > we do in this situation? > > Refcounting abdicates responsibility for ownership management. > It is "more defensive" than managing ownership explicitly, but only insofar as > leaking all allocations is "more defensive" than cleaning up unused garbage ;) > > If refcounting is not needed please manage ownership explicitly (via > scoped_ptr). Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_delegate.h:138: std::vector<VABufferID> pending_va_bufs_; On 2013/05/22 23:59:47, Ami Fischman wrote: > On 2013/05/21 22:32:35, posciak wrote: > > On 2013/05/17 23:19:15, Ami Fischman wrote: > > > Do these really need to be separate? > > > (AFAICT they're all fed to the same APIs; does VAAPI require to see > non-slices > > > before slices??) > > > > All the code that used VAAPI that I've seen, including the tests and code > > written by Intel VAAPI developers does the same thing. Even if it was possible > > now, who knows when this might break. I'd prefer not to go against the flow > > here, as much as I don't like this separation myself either. > > You just gave a great definition of what it means to cargo-cult. Not exactly. I'm saying that even if it was ok to do, it's sadly irrelevant, if everybody else does it this way, if libva/vaapi at some point develops a related bug for this/starts depending on it/stops working otherwise, we will be the only ones that will start exploding for seemingly inexplicable reasons and will most probably waste a mountain of time debugging, staring at raw command buffers sent to the GPU for weeks. Trust me, it wouldn't be the first time. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... File content/common/gpu/media/vaapi_h264_decoder.cc (right): https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.cc:31: VASurfaceID va_surface_id() { On 2013/05/22 23:59:47, Ami Fischman wrote: > On 2013/05/21 22:32:35, posciak wrote: > > On 2013/05/17 23:19:15, Ami Fischman wrote: > > > Is this really adding value compared with va_surface()->id() ? > > > > Avoid a second call? I bet you'll say this is not a reason enough... > > lol I take it as lol ok. Another reason: va_surface() returns scoped_refptr. Let's keep this simple. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.cc:1005: if (pic->outputted) On 2013/05/22 23:59:47, Ami Fischman wrote: > On 2013/05/21 22:32:35, posciak wrote: > > On 2013/05/17 23:19:15, Ami Fischman wrote: > > > how can this happen? > > > > Another visitor from the future, but a harmless one. Let's keep it. > > That is a terrible logic. Head-scratchers should always have a comment if they > can't be deleted. Done. https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... File content/common/gpu/media/vaapi_h264_decoder.h (right): https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.h:42: enum VAVDAH264DecoderFailure { On 2013/05/22 23:59:47, Ami Fischman wrote: > On 2013/05/21 22:32:35, posciak wrote: > > On 2013/05/17 23:19:15, Ami Fischman wrote: > > > s/VAVDA/VAAPI/ ? > > > > All errors but VAAPI_ERROR are actually VAVDA-specific. > > My point was that VaapiH264Decoder is a thing, and VaapiVDA is a thing, but this > enum name combines "VAVDA" and "H264Decoder", which are not a thing. The name is already out there in histograms since a few weeks ago. Could we not change it now please? https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.h:100: DecResult DecodeOneFrame(int32 input_id) WARN_UNUSED_RESULT; On 2013/05/22 23:59:47, Ami Fischman wrote: > On 2013/05/21 22:32:35, posciak wrote: > > On 2013/05/17 23:19:15, Ami Fischman wrote: > > > should |input_id| (here and in DecodeInitial) actually be a param to > > > SetStream()? > > > > This assumes knowledge of VDA specifics that it splits stream into chunks that > > produce at most one frame. > > I don't see that, plus it's not true (a single NALU can complete multiple frames > that were coded relative to it). Completing is not decoding, as decode order != output order. And input ids are not assigned in output order. And what you say is not exactly valid either, you could argue the same for SetStream(): data in one SetStream() could trigger completion of multiple frames decoded as the result of previous SetStream() calls as well. > > > I don't want to have this constraint here. You should be able to SetStream() > for > > more than one frame, and call DecodeOneFrame() with the input_id you want for > > the frame that comes next in that stream buffer. > > On the contrary, I believe that this API implies that the decoder's client knows > where frame boundaries live. In reality input_id is 1:1 to only input buffers, > which are the inputs to SetStream(), so ISTM that SetStream is the place where > input_ids should be specified. a) taking into account what I said above, I don't believe SetStream to be a better place for input_id than this method; b) and I'm still arguing this is the correct place for it, because unlike SetStream, this function will decode exactly one frame - possibly outputting multiple, or possibly requesting more SetStream()s before it returns - and input_id will be assigned to that exact frame. No other ids will be assigned as a result of this single call (which may not be true in case of SetStream, which may result in decoding, not outputting, zero, one or more frames). What we say with this call is: "Continue decoding the stream that I gave you some time ago via SetStream(), from whatever position you currently are in it (and keep asking me for more SetStream()s if you don't have enough) and assign this input_id to the next frame in decoding order your find. At some point in the future (possibly after more SetStream() calls) you will decide to output this frame. When you do, pass me this input_id with it". https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/... content/common/gpu/media/vaapi_video_decode_accelerator.cc:19: namespace { On 2013/05/22 23:59:47, Ami Fischman wrote: > On 2013/05/21 22:32:35, posciak wrote: > > On 2013/05/17 23:19:15, Ami Fischman wrote: > > > pointless > > > > Even if at some point a global ReportToUMA() surfaces? > > Anything global in the future should be namespaced, so yes. Done. https://chromiumcodereview.appspot.com/14914009/diff/12001/content/common/gpu... File content/common/gpu/media/vaapi_delegate.cc (right): https://chromiumcodereview.appspot.com/14914009/diff/12001/content/common/gpu... content/common/gpu/media/vaapi_delegate.cc:125: VAAPI_SYM(DestroyBuffer, vaapi_handle); On 2013/05/22 23:59:47, Ami Fischman wrote: > Please lexicographically sort the 3 lists involved w/ these sym's. Done. https://chromiumcodereview.appspot.com/14914009/diff/12001/content/common/gpu... content/common/gpu/media/vaapi_delegate.cc:224: DVLOG(1) << "YUV420 not supported"; On 2013/05/23 03:02:55, marcheu wrote: > nit: that message is a bit hard to grasp at first, could it be something like > "The vaapi implementation doesn't support YUV420"? Done. https://chromiumcodereview.appspot.com/14914009/diff/12001/content/common/gpu... File content/common/gpu/media/vaapi_delegate.h (right): https://chromiumcodereview.appspot.com/14914009/diff/12001/content/common/gpu... content/common/gpu/media/vaapi_delegate.h:32: class VaapiDelegate : public base::RefCountedThreadSafe<VaapiDelegate> { On 2013/05/22 23:59:47, Ami Fischman wrote: > please rename to wrapper. Delegate means a different thing to wrapper (if > anything, FooDelegate would be the thing that a FooWrapper wraps, as it is the > thing that is dispatched to, not the thing doing the dispatching). Done. Lunch topic next time I visit KIR? ;) https://chromiumcodereview.appspot.com/14914009/diff/12001/content/common/gpu... content/common/gpu/media/vaapi_delegate.h:34: // |report_error_to_uma_cb| will be used to report errors for UMA purposes, On 2013/05/22 23:59:47, Ami Fischman wrote: > nit: with the gloriously informative new name, this comment is less necessary. Done. https://chromiumcodereview.appspot.com/14914009/diff/12001/content/common/gpu... content/common/gpu/media/vaapi_delegate.h:67: // Execute decode in hardware and destroy pending buffers. On 2013/05/22 23:59:47, Ami Fischman wrote: > what does a false return value mean? more data necessary? stream error? etc. Done. https://chromiumcodereview.appspot.com/14914009/diff/12001/content/common/gpu... content/common/gpu/media/vaapi_delegate.h:75: gfx::Size dest_size); On 2013/05/22 23:59:47, Ami Fischman wrote: > Is it ever the case that the pixmap & the surface have different sizes? Not for us, but it's allowed, it'd be scaled then. https://chromiumcodereview.appspot.com/14914009/diff/12001/content/common/gpu... content/common/gpu/media/vaapi_delegate.h:90: bool SubmitDecode(VASurfaceID va_surface_id); On 2013/05/22 23:59:47, Ami Fischman wrote: > ditto q about meaning of return false. Done. https://chromiumcodereview.appspot.com/14914009/diff/12001/content/common/gpu... content/common/gpu/media/vaapi_delegate.h:125: On 2013/05/22 23:59:47, Ami Fischman wrote: > nit: add an extra \n I think. Moved out. https://chromiumcodereview.appspot.com/14914009/diff/12001/content/common/gpu... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/14914009/diff/12001/content/common/gpu... content/common/gpu/media/vaapi_video_decode_accelerator.cc:80: const GLXFBConfig& fb_config, On 2013/05/22 23:59:47, Ami Fischman wrote: > I was asking for the members, accessors, params to Create, and params to ctor be > in the same order. I don't really care which order, as long as they all match. Done. https://chromiumcodereview.appspot.com/14914009/diff/12001/content/common/gpu... content/common/gpu/media/vaapi_video_decode_accelerator.cc:215: DCHECK(glx_pixmap_); On 2013/05/23 03:02:55, marcheu wrote: > Maybe here CHECK/DCHECK that you're on the main thread? And probably in the > other functions which use X/glX as well. I didn't want to pass message loops to TFPPicture. I do check for that before calling TFPPicture methods however. https://chromiumcodereview.appspot.com/14914009/diff/12001/content/content_co... File content/content_common.gypi (right): https://chromiumcodereview.appspot.com/14914009/diff/12001/content/content_co... content/content_common.gypi:453: '<(DEPTH)/third_party/khronos', On 2013/05/22 23:59:47, Ami Fischman wrote: > Argh please don't rebase & address comments in the same upload! > Address comments first, upload, then rebase & upload new patchset. Sorry. https://chromiumcodereview.appspot.com/14914009/diff/12001/media/filters/gpu_... File media/filters/gpu_video_decoder.cc (right): https://chromiumcodereview.appspot.com/14914009/diff/12001/media/filters/gpu_... media/filters/gpu_video_decoder.cc:378: //DCHECK_EQ(status != DemuxerStream::kOk, !buffer) << status; On 2013/05/22 23:59:47, Ami Fischman wrote: > lolwat? Planning to revert this file? lol, yeah sorry
Responding to some comments in email (not rietveld) b/c of file rename. > https://chromiumcodereview.**appspot.com/14914009/diff/** > 3001/content/common/gpu/media/**vaapi_delegate.cc#newcode273<https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/media/vaapi_delegate.cc#newcode273> > content/common/gpu/media/**vaapi_delegate.cc:273: DestroySurfaces(); > On 2013/05/22 23:59:47, Ami Fischman wrote: > >> On 2013/05/21 22:32:35, posciak wrote: >> > On 2013/05/17 23:19:15, Ami Fischman wrote: >> > > I think this isn't really necessary, in which case inline >> > DestroySurfaces > >> into >> > > the dtor. >> > >> > Please see .h about inlining DestroySurfaces(). >> > I'd prefer keeping it this way, doesn't really make a big >> > difference, but at > >> > least releases us from having a contract that you can't do anything >> > with the > >> > class after this fails. >> > > Why would you not want to have that contract? >> > > I prefer not having contracts when they can be avoided, even if they are > very unlikely (well, that is actually another argument for not having > the contract). > I'm not sure what are we disagreeing here about, is it to save us this > one line of code? The contract we're talking about is a /restriction/ - lifting it later is trivial (no caller will need to be modified). Omitting the contract and implying (via the API) that the use case of Destroy+Create is supported is the (implicit) contract you're offering now, which is unnecessary b/c no callers need it. What we're disagreeing about is API surface size - I'm saying offer the smallest surface you can get away with, and you're saying "but it's easy to offer this wider surface" and I'm saying "but that'll be trouble in the future, and is unneeded". > https://chromiumcodereview.**appspot.com/14914009/diff/** > 3001/content/common/gpu/media/**vaapi_delegate.h#newcode138<https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/media/vaapi_delegate.h#newcode138> > content/common/gpu/media/**vaapi_delegate.h:138: std::vector<VABufferID> > pending_va_bufs_; > On 2013/05/22 23:59:47, Ami Fischman wrote: > >> On 2013/05/21 22:32:35, posciak wrote: >> > On 2013/05/17 23:19:15, Ami Fischman wrote: >> > > Do these really need to be separate? >> > > (AFAICT they're all fed to the same APIs; does VAAPI require to >> > see > >> non-slices >> > > before slices??) >> > >> > All the code that used VAAPI that I've seen, including the tests and >> > code > >> > written by Intel VAAPI developers does the same thing. Even if it >> > was possible > >> > now, who knows when this might break. I'd prefer not to go against >> > the flow > >> > here, as much as I don't like this separation myself either. >> > > You just gave a great definition of what it means to cargo-cult. >> > > Not exactly. I'm saying that even if it was ok to do, it's sadly > irrelevant, if everybody else does it this way, if libva/vaapi at some > point develops a related bug for this/starts depending on it/stops > working otherwise, we will be the only ones that will start exploding > for seemingly inexplicable reasons and will most probably waste a > mountain of time debugging, staring at raw command buffers sent to the > GPU for weeks. Trust me, it wouldn't be the first time. :( but I yield. > https://chromiumcodereview.**appspot.com/14914009/diff/** > 3001/content/common/gpu/media/**vaapi_h264_decoder.cc<https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/media/vaapi_h264_decoder.cc> > File content/common/gpu/media/**vaapi_h264_decoder.cc (right): > > https://chromiumcodereview.**appspot.com/14914009/diff/** > 3001/content/common/gpu/media/**vaapi_h264_decoder.cc#**newcode31<https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/media/vaapi_h264_decoder.cc#newcode31> > content/common/gpu/media/**vaapi_h264_decoder.cc:31: VASurfaceID > va_surface_id() { > On 2013/05/22 23:59:47, Ami Fischman wrote: > >> On 2013/05/21 22:32:35, posciak wrote: >> > On 2013/05/17 23:19:15, Ami Fischman wrote: >> > > Is this really adding value compared with va_surface()->id() ? >> > >> > Avoid a second call? I bet you'll say this is not a reason enough... > > lol > > I take it as lol ok. > I meant that the reason you gave is laughable. > Another reason: va_surface() returns scoped_refptr. Let's keep this > simple. For "efficiency" . Again, this is not how I'd do it, but I don't want to keep arguing about it. https://chromiumcodereview.**appspot.com/14914009/diff/** > 3001/content/common/gpu/media/**vaapi_h264_decoder.h<https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/media/vaapi_h264_decoder.h> > File content/common/gpu/media/**vaapi_h264_decoder.h (right): > > https://chromiumcodereview.**appspot.com/14914009/diff/** > 3001/content/common/gpu/media/**vaapi_h264_decoder.h#newcode42<https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/media/vaapi_h264_decoder.h#newcode42> > content/common/gpu/media/**vaapi_h264_decoder.h:42: enum > VAVDAH264DecoderFailure { > On 2013/05/22 23:59:47, Ami Fischman wrote: > >> On 2013/05/21 22:32:35, posciak wrote: >> > On 2013/05/17 23:19:15, Ami Fischman wrote: >> > > s/VAVDA/VAAPI/ ? >> > >> > All errors but VAAPI_ERROR are actually VAVDA-specific. >> > > My point was that VaapiH264Decoder is a thing, and VaapiVDA is a >> > thing, but this > >> enum name combines "VAVDA" and "H264Decoder", which are not a thing. > > The name is already out there in histograms since a few weeks ago. Could > we not change it now please? I guess. > > > https://chromiumcodereview.**appspot.com/14914009/diff/** > 3001/content/common/gpu/media/**vaapi_h264_decoder.h#**newcode100<https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/media/vaapi_h264_decoder.h#newcode100> > content/common/gpu/media/**vaapi_h264_decoder.h:100: DecResult > DecodeOneFrame(int32 input_id) WARN_UNUSED_RESULT; > On 2013/05/22 23:59:47, Ami Fischman wrote: > >> On 2013/05/21 22:32:35, posciak wrote: >> > On 2013/05/17 23:19:15, Ami Fischman wrote: >> > > should |input_id| (here and in DecodeInitial) actually be a param >> > to > >> > > SetStream()? >> > >> > This assumes knowledge of VDA specifics that it splits stream into >> > chunks that > >> > produce at most one frame. >> > > I don't see that, plus it's not true (a single NALU can complete >> > multiple frames > >> that were coded relative to it). >> > > Completing is not decoding, as decode order != output order. And input > ids are not assigned in output order. > And what you say is not exactly valid either, you could argue the same > for SetStream(): data in one SetStream() could trigger completion of > multiple frames decoded as the result of previous SetStream() calls as > well. Yes, but this is a limitation of the API we're using (VAAPI). Please think about this with an open mind. If you still think this is the better choice, fine. > > > https://chromiumcodereview.**appspot.com/14914009/diff/** > 12001/content/common/gpu/**media/vaapi_delegate.h<https://chromiumcodereview.appspot.com/14914009/diff/12001/content/common/gpu/media/vaapi_delegate.h> > File content/common/gpu/media/**vaapi_delegate.h (right): > > https://chromiumcodereview.**appspot.com/14914009/diff/** > 12001/content/common/gpu/**media/vaapi_delegate.h#**newcode32<https://chromiumcodereview.appspot.com/14914009/diff/12001/content/common/gpu/media/vaapi_delegate.h#newcode32> > content/common/gpu/media/**vaapi_delegate.h:32: class VaapiDelegate : > public base::RefCountedThreadSafe<**VaapiDelegate> { > On 2013/05/22 23:59:47, Ami Fischman wrote: > >> please rename to wrapper. Delegate means a different thing to wrapper >> > (if > >> anything, FooDelegate would be the thing that a FooWrapper wraps, as >> > it is the > >> thing that is dispatched to, not the thing doing the dispatching). >> > > Done. Lunch topic next time I visit KIR? ;) Only on a Tuesday. Otherwise I'll be in SEA. > > https://chromiumcodereview.**appspot.com/14914009/diff/** > 12001/content/common/gpu/**media/vaapi_delegate.h#**newcode75<https://chromiumcodereview.appspot.com/14914009/diff/12001/content/common/gpu/media/vaapi_delegate.h#newcode75> > content/common/gpu/media/**vaapi_delegate.h:75: gfx::Size dest_size); > On 2013/05/22 23:59:47, Ami Fischman wrote: > >> Is it ever the case that the pixmap & the surface have different >> > sizes? > > Not for us, but it's allowed, it'd be scaled then. > >
https://chromiumcodereview.appspot.com/14914009/diff/34013/content/common/gpu... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/14914009/diff/34013/content/common/gpu... content/common/gpu/media/vaapi_video_decode_accelerator.cc:211: DCHECK(glx_pixmap_); On 2013/05/24 01:46:39, posciak wrote: > On 2013/05/23 03:02:55, marcheu wrote: > > Maybe here CHECK/DCHECK that you're on the main thread? And probably in the > > other functions which use X/glX as well. > > I didn't want to pass message loops to TFPPicture. I do check for that before > calling TFPPicture methods however. You don't need to pass loops to DCHECK being on the ChildThread b/c there's a ChildThread static accessor you can call from any thread. https://chromiumcodereview.appspot.com/14914009/diff/34013/content/common/gpu... File content/common/gpu/media/vaapi_wrapper.h (right): https://chromiumcodereview.appspot.com/14914009/diff/34013/content/common/gpu... content/common/gpu/media/vaapi_wrapper.h:71: // buffers. Return false if SubmitDecode() fails. Does this include the "need more data" case? Or is it the case that at this layer, "need more data" is actually a fatal error?
On 2013/05/24 02:11:11, Ami Fischman wrote: > Responding to some comments in email (not rietveld) b/c of file rename. > > > > https://chromiumcodereview.**appspot.com/14914009/diff/** > > > 3001/content/common/gpu/media/**vaapi_delegate.cc#newcode273<https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/media/vaapi_delegate.cc#newcode273> > > content/common/gpu/media/**vaapi_delegate.cc:273: DestroySurfaces(); > > On 2013/05/22 23:59:47, Ami Fischman wrote: > > > >> On 2013/05/21 22:32:35, posciak wrote: > >> > On 2013/05/17 23:19:15, Ami Fischman wrote: > >> > > I think this isn't really necessary, in which case inline > >> > > DestroySurfaces > > > >> into > >> > > the dtor. > >> > > >> > Please see .h about inlining DestroySurfaces(). > >> > I'd prefer keeping it this way, doesn't really make a big > >> > > difference, but at > > > >> > least releases us from having a contract that you can't do anything > >> > > with the > > > >> > class after this fails. > >> > > > > Why would you not want to have that contract? > >> > > > > I prefer not having contracts when they can be avoided, even if they are > > very unlikely (well, that is actually another argument for not having > > the contract). > > I'm not sure what are we disagreeing here about, is it to save us this > > one line of code? > > > The contract we're talking about is a /restriction/ - lifting it later is > trivial (no caller will need to be modified). Omitting the contract and > implying (via the API) that the use case of Destroy+Create is supported is > the (implicit) contract you're offering now, which is unnecessary b/c no > callers need it. What we're disagreeing about is API surface size - I'm > saying offer the smallest surface you can get away with, and you're saying > "but it's easy to offer this wider surface" and I'm saying "but that'll be > trouble in the future, and is unneeded". > Wait, I feel like we are talking about two different things now. We were discussing whether or not to allow another call to CreateSurfaces() after it failed once, which I'm ok not to have. But now you seem to be against allowing the sequence of Create -> Destroy -> Create (all successful), which is important and needed by clients for config changes. Are you? > > > https://chromiumcodereview.**appspot.com/14914009/diff/** > > > 3001/content/common/gpu/media/**vaapi_delegate.h#newcode138<https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/media/vaapi_delegate.h#newcode138> > > content/common/gpu/media/**vaapi_delegate.h:138: std::vector<VABufferID> > > pending_va_bufs_; > > On 2013/05/22 23:59:47, Ami Fischman wrote: > > > >> On 2013/05/21 22:32:35, posciak wrote: > >> > On 2013/05/17 23:19:15, Ami Fischman wrote: > >> > > Do these really need to be separate? > >> > > (AFAICT they're all fed to the same APIs; does VAAPI require to > >> > > see > > > >> non-slices > >> > > before slices??) > >> > > >> > All the code that used VAAPI that I've seen, including the tests and > >> > > code > > > >> > written by Intel VAAPI developers does the same thing. Even if it > >> > > was possible > > > >> > now, who knows when this might break. I'd prefer not to go against > >> > > the flow > > > >> > here, as much as I don't like this separation myself either. > >> > > > > You just gave a great definition of what it means to cargo-cult. > >> > > > > Not exactly. I'm saying that even if it was ok to do, it's sadly > > irrelevant, if everybody else does it this way, if libva/vaapi at some > > point develops a related bug for this/starts depending on it/stops > > working otherwise, we will be the only ones that will start exploding > > for seemingly inexplicable reasons and will most probably waste a > > mountain of time debugging, staring at raw command buffers sent to the > > GPU for weeks. Trust me, it wouldn't be the first time. > > > :( but I yield. > Thanks and sorry, I really wanted to unify it too, but decided against it, I don't want to risk it. > > > > https://chromiumcodereview.**appspot.com/14914009/diff/** > > > 3001/content/common/gpu/media/**vaapi_h264_decoder.cc<https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/media/vaapi_h264_decoder.cc> > > File content/common/gpu/media/**vaapi_h264_decoder.cc (right): > > > > https://chromiumcodereview.**appspot.com/14914009/diff/** > > > 3001/content/common/gpu/media/**vaapi_h264_decoder.cc#**newcode31<https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/media/vaapi_h264_decoder.cc#newcode31> > > content/common/gpu/media/**vaapi_h264_decoder.cc:31: VASurfaceID > > va_surface_id() { > > On 2013/05/22 23:59:47, Ami Fischman wrote: > > > >> On 2013/05/21 22:32:35, posciak wrote: > >> > On 2013/05/17 23:19:15, Ami Fischman wrote: > >> > > Is this really adding value compared with va_surface()->id() ? > >> > > >> > Avoid a second call? I bet you'll say this is not a reason enough... > > > > lol > > > > I take it as lol ok. > > > > I meant that the reason you gave is laughable. > > > > Another reason: va_surface() returns scoped_refptr. Let's keep this > > simple. > > > For "efficiency" . > > Again, this is not how I'd do it, but I don't want to keep arguing about it. > I'd love to hear what you'd do though... I am obsessed with making all this nice and clean, I am a refactoring and code beauty junkie. Would you have a different getter for the refptr version? > https://chromiumcodereview.**appspot.com/14914009/diff/** > > > 3001/content/common/gpu/media/**vaapi_h264_decoder.h<https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/media/vaapi_h264_decoder.h> > > File content/common/gpu/media/**vaapi_h264_decoder.h (right): > > > > https://chromiumcodereview.**appspot.com/14914009/diff/** > > > 3001/content/common/gpu/media/**vaapi_h264_decoder.h#newcode42<https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/media/vaapi_h264_decoder.h#newcode42> > > content/common/gpu/media/**vaapi_h264_decoder.h:42: enum > > VAVDAH264DecoderFailure { > > On 2013/05/22 23:59:47, Ami Fischman wrote: > > > >> On 2013/05/21 22:32:35, posciak wrote: > >> > On 2013/05/17 23:19:15, Ami Fischman wrote: > >> > > s/VAVDA/VAAPI/ ? > >> > > >> > All errors but VAAPI_ERROR are actually VAVDA-specific. > >> > > > > My point was that VaapiH264Decoder is a thing, and VaapiVDA is a > >> > > thing, but this > > > >> enum name combines "VAVDA" and "H264Decoder", which are not a thing. > > > > The name is already out there in histograms since a few weeks ago. Could > > we not change it now please? > > > I guess. > Thank you. You are right, but since since it's in the wild already... > > > > > > > https://chromiumcodereview.**appspot.com/14914009/diff/** > > > 3001/content/common/gpu/media/**vaapi_h264_decoder.h#**newcode100<https://chromiumcodereview.appspot.com/14914009/diff/3001/content/common/gpu/media/vaapi_h264_decoder.h#newcode100> > > content/common/gpu/media/**vaapi_h264_decoder.h:100: DecResult > > DecodeOneFrame(int32 input_id) WARN_UNUSED_RESULT; > > On 2013/05/22 23:59:47, Ami Fischman wrote: > > > >> On 2013/05/21 22:32:35, posciak wrote: > >> > On 2013/05/17 23:19:15, Ami Fischman wrote: > >> > > should |input_id| (here and in DecodeInitial) actually be a param > >> > > to > > > >> > > SetStream()? > >> > > >> > This assumes knowledge of VDA specifics that it splits stream into > >> > > chunks that > > > >> > produce at most one frame. > >> > > > > I don't see that, plus it's not true (a single NALU can complete > >> > > multiple frames > > > >> that were coded relative to it). > >> > > > > Completing is not decoding, as decode order != output order. And input > > ids are not assigned in output order. > > And what you say is not exactly valid either, you could argue the same > > for SetStream(): data in one SetStream() could trigger completion of > > multiple frames decoded as the result of previous SetStream() calls as > > well. > > > Yes, but this is a limitation of the API we're using (VAAPI). > Please think about this with an open mind. If you still think this is the > better choice, fine. > I spent a good deal of time thinking about it and I'm unhappy about both: The ideal API I imagined when I was designing the decoder was to SetStream() and keep Decode()ing without returning until we ran out, then we'd SetStream() again and keep going. And it should be allowed for each stream chunk to have any (0-many) number of frames in decoding order. But our API doesn't allow this, because we can pass down only one input_id per SetStream. So you are right, with the VDA API we have, we can't really achieve this. I wish we could just get a starting input id and use subsequent ones automatically for the duration of one SetStream(). Your approach would still be right then. Anyway, I've been working on more changes in parallel to this for some time now and it's likely I'll be getting rid of SetStream and making Decode() take each stream chunk and consume the whole thing in one run and return, like EVDA does, whatever the result might be, which implies only one actual decode per stream chunk, which is the VDA contract anyway. Also I want to have one task per Decode() to get rid of locking on decoder thread. This would also be basically what you are proposing. Can we have that change in the next CL along with other changes I'm mentioning here? [cutting out the remainder due to rietveld's msg length limit]
https://chromiumcodereview.appspot.com/14914009/diff/34013/content/common/gpu... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/14914009/diff/34013/content/common/gpu... content/common/gpu/media/vaapi_video_decode_accelerator.cc:211: DCHECK(glx_pixmap_); On 2013/05/24 02:14:37, Ami Fischman wrote: > On 2013/05/24 01:46:39, posciak wrote: > > On 2013/05/23 03:02:55, marcheu wrote: > > > Maybe here CHECK/DCHECK that you're on the main thread? And probably in the > > > other functions which use X/glX as well. > > > > I didn't want to pass message loops to TFPPicture. I do check for that before > > calling TFPPicture methods however. > > You don't need to pass loops to DCHECK being on the ChildThread b/c there's a > ChildThread static accessor you can call from any thread. Oh nice, thanks, done. https://chromiumcodereview.appspot.com/14914009/diff/34013/content/common/gpu... File content/common/gpu/media/vaapi_wrapper.h (right): https://chromiumcodereview.appspot.com/14914009/diff/34013/content/common/gpu... content/common/gpu/media/vaapi_wrapper.h:71: // buffers. Return false if SubmitDecode() fails. On 2013/05/24 02:14:37, Ami Fischman wrote: > Does this include the "need more data" case? > Or is it the case that at this layer, "need more data" is actually a fatal > error? The client is expected to provide the exact amount of data needed to decode a frame. If it wasn't capable of doing so, providing additional data and retrying is not possible, but it's possible to restart from the beginning of a frame, providing all the data again for the frame, of course this time proper data, or just skip it and continue from another frame, but for that you'd have to ensure that that other frame wouldn't require the failed one to decode correctly. We treat this as fatal failure and not recover from it, in practice it doesn't really happen, unless the stream is badly corrupted, most of the time corruption just results in artifacts and Decode doesn't fail. There are quite a lot of different things that can go wrong in decode: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libva/.... Even if it failed, we'd need to do recovery from another, safe point of the stream. I can add much more documentation about this, but to be honest, any client using this class needs to have deep knowledge of VAAPI anyway, even to be able to submit buffers properly. I'm not sure it makes sense to rewrite that here... It's just a wrapper after all ;)
Added DCHECKs, responded to previous comments in previous reply.
lgtm https://codereview.chromium.org/14914009/diff/55001/content/common/gpu/media/... File content/common/gpu/media/vaapi_h264_decoder.cc (right): https://codereview.chromium.org/14914009/diff/55001/content/common/gpu/media/... content/common/gpu/media/vaapi_h264_decoder.cc:31: return va_surface_->id(); > > > > > > > > https://chromiumcodereview.**appspot.com/14914009/diff/** > > > > > > 3001/content/common/gpu/media/**vaapi_h264_decoder.cc<<crxref style="background-image: url(chrome-extension://ppnlfijacbhfpjgkafbeiaklmgoljooo/res/type/codereview.png);">https://chromiumcodereview.appspot.com/14914009/</crxref>diff/3001/content/common/gpu/media/vaapi_h264_decoder.cc> > > > File content/common/gpu/media/**vaapi_h264_decoder.cc (right): > > > > > > https://chromiumcodereview.**appspot.com/14914009/diff/** > > > > > > 3001/content/common/gpu/media/**vaapi_h264_decoder.cc#**newcode31<<crxref style="background-image: url(chrome-extension://ppnlfijacbhfpjgkafbeiaklmgoljooo/res/type/codereview.png);">https://chromiumcodereview.appspot.com/14914009/</crxref>diff/3001/content/common/gpu/media/vaapi_h264_decoder.cc#newcode31> > > > content/common/gpu/media/**vaapi_h264_decoder.cc:31: VASurfaceID > > > va_surface_id() { > > > On 2013/05/22 23:59:47, Ami Fischman wrote: > > > > > >> On 2013/05/21 22:32:35, posciak wrote: > > >> > On 2013/05/17 23:19:15, Ami Fischman wrote: > > >> > > Is this really adding value compared with va_surface()->id() ? > > >> > > > >> > Avoid a second call? I bet you'll say this is not a reason enough... > > > > > > lol > > > > > > I take it as lol ok. > > > > > > > I meant that the reason you gave is laughable. > > > > > > > Another reason: va_surface() returns scoped_refptr. Let's keep this > > > simple. > > > > > > For "efficiency" . > > > > Again, this is not how I'd do it, but I don't want to keep arguing about it. > > > > I'd love to hear what you'd do though... I am obsessed with making all this nice > and clean, I am a refactoring and code beauty junkie. Would you have a different > getter for the refptr version? I would simply elide this getter and replace its callers with va_surface()->id() instead. https://codereview.chromium.org/14914009/diff/55001/content/common/gpu/media/... File content/common/gpu/media/vaapi_h264_decoder.h (right): https://codereview.chromium.org/14914009/diff/55001/content/common/gpu/media/... content/common/gpu/media/vaapi_h264_decoder.h:103: DecResult DecodeOneFrame(int32 input_id) WARN_UNUSED_RESULT; > > >> > > should |input_id| (here and in DecodeInitial) actually be a param > > >> > > > to > > > > > >> > > SetStream()? > > >> > > > >> > This assumes knowledge of VDA specifics that it splits stream into > > >> > > > chunks that > > > > > >> > produce at most one frame. > > >> > > > > > > I don't see that, plus it's not true (a single NALU can complete > > >> > > > multiple frames > > > > > >> that were coded relative to it). > > >> > > > > > > Completing is not decoding, as decode order != output order. And input > > > ids are not assigned in output order. > > > And what you say is not exactly valid either, you could argue the same > > > for SetStream(): data in one SetStream() could trigger completion of > > > multiple frames decoded as the result of previous SetStream() calls as > > > well. > > > > > > Yes, but this is a limitation of the API we're using (VAAPI). > > Please think about this with an open mind. If you still think this is the > > better choice, fine. > > > > I spent a good deal of time thinking about it and I'm unhappy about both: > The ideal API I imagined when I was designing the decoder was to SetStream() and > keep Decode()ing without returning until we ran out, then we'd SetStream() again > and keep going. And it should be allowed for each stream chunk to have any > (0-many) number of frames in decoding order. But our API doesn't allow this, > because we can pass down only one input_id per SetStream. > > So you are right, with the VDA API we have, we can't really achieve this. I wish > we could just get a starting input id and use subsequent ones automatically for > the duration of one SetStream(). Your approach would still be right then. > > Anyway, I've been working on more changes in parallel to this for some time now > and it's likely I'll be getting rid of SetStream and making Decode() take each > stream chunk and consume the whole thing in one run and return, like EVDA does, > whatever the result might be, which implies only one actual decode per stream > chunk, which is the VDA contract anyway. Also I want to have one task per > Decode() to get rid of locking on decoder thread. This would also be basically > what you are proposing. Can we have that change in the next CL along with other > changes I'm mentioning here? Yes. https://codereview.chromium.org/14914009/diff/55001/content/common/gpu/media/... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://codereview.chromium.org/14914009/diff/55001/content/common/gpu/media/... content/common/gpu/media/vaapi_video_decode_accelerator.cc:203: ChildThread::current()->message_loop()); FWIW equiv you can do: DCHECK(ChildThread::current()->message_loop()->message_loop_proxy()->BelongsToCurrentThread()); but this wouldn't help with line wrapping anyway. https://codereview.chromium.org/14914009/diff/55001/content/common/gpu/media/... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/14914009/diff/55001/content/common/gpu/media/... content/common/gpu/media/vaapi_wrapper.cc:289: void VaapiWrapper::DestroySurfaces() { > > > content/common/gpu/media/**vaapi_delegate.cc:273: DestroySurfaces(); > > > On 2013/05/22 23:59:47, Ami Fischman wrote: > > >> On 2013/05/21 22:32:35, posciak wrote: > > >> > On 2013/05/17 23:19:15, Ami Fischman wrote: > > >> > > I think this isn't really necessary, in which case inline DestroySurfaces into > > >> > > the dtor. > > >> > Please see .h about inlining DestroySurfaces(). > > >> > I'd prefer keeping it this way, doesn't really make a big difference, but at > > >> > least releases us from having a contract that you can't do anything with the > > >> > class after this fails. > > > Why would you not want to have that contract? > > > I prefer not having contracts when they can be avoided, even if they are > > > very unlikely (well, that is actually another argument for not having > > > the contract). > > > I'm not sure what are we disagreeing here about, is it to save us this > > > one line of code? > > The contract we're talking about is a /restriction/ - lifting it later is > > trivial (no caller will need to be modified). Omitting the contract and > > implying (via the API) that the use case of Destroy+Create is supported is > > the (implicit) contract you're offering now, which is unnecessary b/c no > > callers need it. What we're disagreeing about is API surface size - I'm > > saying offer the smallest surface you can get away with, and you're saying > > "but it's easy to offer this wider surface" and I'm saying "but that'll be > > trouble in the future, and is unneeded". > Wait, I feel like we are talking about two different things now. We were > discussing whether or not to allow another call to CreateSurfaces() after it > failed once, which I'm ok not to have. > But now you seem to be against allowing the sequence of Create -> Destroy -> > Create (all successful), which is important and needed by clients for config > changes. Are you? I was saying that until you have a need for an API, offering it is unwise. Since VAVDA doesn't support config changes now, that API is unneeded. If you want to offer it now because you feel config changes is right around the corner, then fine. (my original objection was mostly rooted in not realizing you need DestroySurfaces() to support config change, and thus in not seeing what possible use this API would have in the near future)
On 2013/05/29 19:59:04, Ami Fischman wrote: > lgtm > > https://codereview.chromium.org/14914009/diff/55001/content/common/gpu/media/... > File content/common/gpu/media/vaapi_h264_decoder.cc (right): > > https://codereview.chromium.org/14914009/diff/55001/content/common/gpu/media/... > content/common/gpu/media/vaapi_h264_decoder.cc:31: return va_surface_->id(); > > > > > > > > > > > > > https://chromiumcodereview.**appspot.com/14914009/diff/** > > > > > > > > > 3001/content/common/gpu/media/**vaapi_h264_decoder.cc<<crxref > style="background-image: > url(chrome-extension://ppnlfijacbhfpjgkafbeiaklmgoljooo/res/type/codereview.png);">https://chromiumcodereview.appspot.com/14914009/</crxref>diff/3001/content/common/gpu/media/vaapi_h264_decoder.cc> > > > > File content/common/gpu/media/**vaapi_h264_decoder.cc (right): > > > > > > > > https://chromiumcodereview.**appspot.com/14914009/diff/** > > > > > > > > > 3001/content/common/gpu/media/**vaapi_h264_decoder.cc#**newcode31<<crxref > style="background-image: > url(chrome-extension://ppnlfijacbhfpjgkafbeiaklmgoljooo/res/type/codereview.png);">https://chromiumcodereview.appspot.com/14914009/</crxref>diff/3001/content/common/gpu/media/vaapi_h264_decoder.cc#newcode31> > > > > content/common/gpu/media/**vaapi_h264_decoder.cc:31: VASurfaceID > > > > va_surface_id() { > > > > On 2013/05/22 23:59:47, Ami Fischman wrote: > > > > > > > >> On 2013/05/21 22:32:35, posciak wrote: > > > >> > On 2013/05/17 23:19:15, Ami Fischman wrote: > > > >> > > Is this really adding value compared with va_surface()->id() ? > > > >> > > > > >> > Avoid a second call? I bet you'll say this is not a reason enough... > > > > > > > > lol > > > > > > > > I take it as lol ok. > > > > > > > > > > I meant that the reason you gave is laughable. > > > > > > > > > > Another reason: va_surface() returns scoped_refptr. Let's keep this > > > > simple. > > > > > > > > > For "efficiency" . > > > > > > Again, this is not how I'd do it, but I don't want to keep arguing about it. > > > > > > > I'd love to hear what you'd do though... I am obsessed with making all this > nice > > and clean, I am a refactoring and code beauty junkie. Would you have a > different > > getter for the refptr version? > > I would simply elide this getter and replace its callers with va_surface()->id() > instead. va_surface() returns a scoped_refptr... I'm guessing you are saying we are ok with the "enormous" performance hit when we do this?
On Wed, May 29, 2013 at 1:25 PM, <posciak@chromium.org> wrote: > va_surface() returns a scoped_refptr... I'm guessing you are saying we are > ok > with the "enormous" performance hit when we do this? > Am I ok with two uncontended lock/unlock pairs and an increment/decrement pair? Yes.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/posciak@chromium.org/14914009/55001
On Wed, May 29, 2013 at 1:27 PM, Ami Fischman <fischman@chromium.org> wrote: > > On Wed, May 29, 2013 at 1:25 PM, <posciak@chromium.org> wrote: > >> va_surface() returns a scoped_refptr... I'm guessing you are saying we >> are ok >> with the "enormous" performance hit when we do this? >> > > Am I ok with two uncontended lock/unlock pairs and an increment/decrement > pair? Yes. > Ok, will fix this in next stage
Antoine: may I have an OWNERS for content/{,gpu} please?
Antoine: may I have an OWNERS for content/{,gpu} please?
Antoine: may I have an OWNERS for content/{,gpu} please?
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
https://chromiumcodereview.appspot.com/14914009/diff/55001/content/common/gpu... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/14914009/diff/55001/content/common/gpu... content/common/gpu/media/vaapi_video_decode_accelerator.cc:172: glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR); These calls look out of place. This affects the "current texture" in the context, which is the last one that was bound. I'm pretty sure at the time you're calling this it's not the right texture that is bound, so you're modifying parameters on some random other texture. Maybe you meant to do these in TFPPicture::Bind after the ScopedTextureBinder.
https://chromiumcodereview.appspot.com/14914009/diff/55001/content/common/gpu... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/14914009/diff/55001/content/common/gpu... content/common/gpu/media/vaapi_video_decode_accelerator.cc:172: glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR); On 2013/05/29 21:10:02, piman wrote: > These calls look out of place. This affects the "current texture" in the > context, which is the last one that was bound. I'm pretty sure at the time > you're calling this it's not the right texture that is bound, so you're > modifying parameters on some random other texture. > > Maybe you meant to do these in TFPPicture::Bind after the ScopedTextureBinder. I should bind the texture here before calling this, thanks for spotting this. But my understanding is that it's enough to call these once per texture? Bind() is called every frame to do glXBindTexImageEXT(), but my understanding is this is not needed more than once, right?
On 2013/05/29 21:18:37, posciak wrote: > https://chromiumcodereview.appspot.com/14914009/diff/55001/content/common/gpu... > File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): > > https://chromiumcodereview.appspot.com/14914009/diff/55001/content/common/gpu... > content/common/gpu/media/vaapi_video_decode_accelerator.cc:172: > glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR); > On 2013/05/29 21:10:02, piman wrote: > > These calls look out of place. This affects the "current texture" in the > > context, which is the last one that was bound. I'm pretty sure at the time > > you're calling this it's not the right texture that is bound, so you're > > modifying parameters on some random other texture. > > > > Maybe you meant to do these in TFPPicture::Bind after the ScopedTextureBinder. > > I should bind the texture here before calling this, thanks for spotting this. > But my understanding is that it's enough to call these once per texture? Correct. > Bind() > is called every frame to do glXBindTexImageEXT(), but my understanding is this > is not needed more than once, right? Correct. Actually, these are set by the compositor anyway. So maybe it isn't useful to set them here?
Removed glTexParameterf calls and va_surface_id(). PTAL. https://chromiumcodereview.appspot.com/14914009/diff/55001/content/common/gpu... File content/common/gpu/media/vaapi_h264_decoder.cc (right): https://chromiumcodereview.appspot.com/14914009/diff/55001/content/common/gpu... content/common/gpu/media/vaapi_h264_decoder.cc:31: return va_surface_->id(); On 2013/05/29 19:59:04, Ami Fischman wrote: > > > > > > > > > > > > > https://chromiumcodereview.**appspot.com/14914009/diff/** > > > > > > > > > 3001/content/common/gpu/media/**vaapi_h264_decoder.cc<<crxref > style="background-image: > url(chrome-extension://ppnlfijacbhfpjgkafbeiaklmgoljooo/res/type/codereview.png);">https://chromiumcodereview.appspot.com/14914009/</crxref>diff/3001/content/common/gpu/media/vaapi_h264_decoder.cc> > > > > File content/common/gpu/media/**vaapi_h264_decoder.cc (right): > > > > > > > > https://chromiumcodereview.**appspot.com/14914009/diff/** > > > > > > > > > 3001/content/common/gpu/media/**vaapi_h264_decoder.cc#**newcode31<<crxref > style="background-image: > url(chrome-extension://ppnlfijacbhfpjgkafbeiaklmgoljooo/res/type/codereview.png);">https://chromiumcodereview.appspot.com/14914009/</crxref>diff/3001/content/common/gpu/media/vaapi_h264_decoder.cc#newcode31> > > > > content/common/gpu/media/**vaapi_h264_decoder.cc:31: VASurfaceID > > > > va_surface_id() { > > > > On 2013/05/22 23:59:47, Ami Fischman wrote: > > > > > > > >> On 2013/05/21 22:32:35, posciak wrote: > > > >> > On 2013/05/17 23:19:15, Ami Fischman wrote: > > > >> > > Is this really adding value compared with va_surface()->id() ? > > > >> > > > > >> > Avoid a second call? I bet you'll say this is not a reason enough... > > > > > > > > lol > > > > > > > > I take it as lol ok. > > > > > > > > > > I meant that the reason you gave is laughable. > > > > > > > > > > Another reason: va_surface() returns scoped_refptr. Let's keep this > > > > simple. > > > > > > > > > For "efficiency" . > > > > > > Again, this is not how I'd do it, but I don't want to keep arguing about it. > > > > > > > I'd love to hear what you'd do though... I am obsessed with making all this > nice > > and clean, I am a refactoring and code beauty junkie. Would you have a > different > > getter for the refptr version? > > I would simply elide this getter and replace its callers with va_surface()->id() > instead. Done.
LGTM for my part.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/posciak@chromium.org/14914009/72001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... 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/posciak@chromium.org/14914009/85003
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... 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/posciak@chromium.org/14914009/85003
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... 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/posciak@chromium.org/14914009/96002
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... 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/posciak@chromium.org/14914009/106001
Message was sent while issue was closed.
Change committed as 203343 |
