Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11076009:
OVDA: Perform an egl sync before reusing a texture. (Closed)
|
Created:
8 years, 2 months ago by Pawel Osciak Modified:
8 years, 2 months ago 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. |
DescriptionOVDA: Perform an EGL sync before reusing a texture.
When a texture is returned via ReusePictureBuffer(), we know
that it has been inserted into command stream, but it may not
have been read out yet.
This inserts an EGL sync object into the GPU command stream.
All commands that have already been in the stream at the time
of insertion are finished when the object is signaled,
thus guaranteeing that the commands to read out the texture
have been finished. The texture will be reused for decoding
after the object has been signaled.
BUG=155602
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161759
Patch Set 1 #Patch Set 2 : LOG()->DLOG() #
Total comments: 14
Patch Set 3 : #
Total comments: 20
Patch Set 4 : #
Total comments: 16
Patch Set 5 : #Patch Set 6 : #
Messages
Total messages: 26 (0 generated)
I don't like paying the extra thread per decoder for this. How long do these sync's take to pass in the common case? Does it make sense to PostDelayedTask a short time in the future, repeatedly, until the sync is signaled? Separately, does it make sense to use ui/gl/gl_fence.h's GLFence::Create(), instead?
https://codereview.chromium.org/11076009/diff/3001/content/common/gpu/media/o... File content/common/gpu/media/omx_video_decode_accelerator.cc (right): https://codereview.chromium.org/11076009/diff/3001/content/common/gpu/media/o... content/common/gpu/media/omx_video_decode_accelerator.cc:42: eglGetProcAddress("eglCreateSyncKHR")); No static initialization. Realize that this code runs in the browser too and is doing I/O during startup. Realize as well that eglGetProcAddress doesn't exist on GL - in fact we dynamically load EGL, so this may be NULL. https://codereview.chromium.org/11076009/diff/3001/content/common/gpu/media/o... content/common/gpu/media/omx_video_decode_accelerator.cc:430: &OmxVideoDecodeAccelerator::WaitForPictureSync, weak_this_, You can't pass a weak pointer to another thread. It cannot be made thread-safe (note: it'll assert in debug). What you can do is put a pointer to the weak pointer into the closure, and call a free function on the thread (as well as other params you need in there). That function can post a task to the main thread where you can then dereference the weak pointer before calling back into the class. https://codereview.chromium.org/11076009/diff/3001/content/common/gpu/media/o... content/common/gpu/media/omx_video_decode_accelerator.cc:438: EGLint ret = egl_client_wait_sync_khr(egl_display_, egl_sync_obj, How do you know it's safe to access egl_display_ without a lock? What if this gets deleted while this function is running? https://codereview.chromium.org/11076009/diff/3001/content/common/gpu/media/o... content/common/gpu/media/omx_video_decode_accelerator.cc:440: kSyncTimeoutNanosec); Why using a timeout at all? A lock with a timeout is like no lock - imagine if you attach the debugger, or if the thread isn't scheduled in time because the CPU was busy doing other things, or whatever. You create races that are hard to repro/debug. If we're worried about blocking the thread for too long, then we should post a task to try again, not return success after the failure. https://codereview.chromium.org/11076009/diff/3001/content/common/gpu/media/o... content/common/gpu/media/omx_video_decode_accelerator.cc:449: message_loop_->PostTask(FROM_HERE, base::Bind( Same here, accessing message_loop_ isn't safe. Instead, pass a message loop proxy to the function. https://codereview.chromium.org/11076009/diff/3001/content/common/gpu/media/o... File content/common/gpu/media/omx_video_decode_accelerator.h (right): https://codereview.chromium.org/11076009/diff/3001/content/common/gpu/media/o... content/common/gpu/media/omx_video_decode_accelerator.h:19: #include "base/threading/thread.h" nit: alpha order
https://codereview.chromium.org/11076009/diff/3001/content/common/gpu/media/o... File content/common/gpu/media/omx_video_decode_accelerator.cc (right): https://codereview.chromium.org/11076009/diff/3001/content/common/gpu/media/o... content/common/gpu/media/omx_video_decode_accelerator.cc:416: EGLSyncKHR egl_sync_obj = egl_create_sync_khr(egl_display_, I'd be concerned about this leaking an EGLSyncKHR if tasks get cancelled somehow. This can't happen now, since we let the class destructor destroy the sync_thread_ (race conditions as piman points out), and all callbacks will eventually be called. If you eventually make these callbacks though, you'll have to wrap the EGLSyncKHR in some kind of ownership-tracked struct to make sure we don't leak them.
https://codereview.chromium.org/11076009/diff/3001/content/common/gpu/media/o... File content/common/gpu/media/omx_video_decode_accelerator.cc (right): https://codereview.chromium.org/11076009/diff/3001/content/common/gpu/media/o... content/common/gpu/media/omx_video_decode_accelerator.cc:416: EGLSyncKHR egl_sync_obj = egl_create_sync_khr(egl_display_, I meant: "if you eventually make these callbacks cancelable though..."
https://chromiumcodereview.appspot.com/11076009/diff/3001/content/common/gpu/... File content/common/gpu/media/omx_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/11076009/diff/3001/content/common/gpu/... content/common/gpu/media/omx_video_decode_accelerator.cc:42: eglGetProcAddress("eglCreateSyncKHR")); On 2012/10/06 06:42:40, piman wrote: > No static initialization. Realize that this code runs in the browser too and is > doing I/O during startup. Realize as well that eglGetProcAddress doesn't exist > on GL - in fact we dynamically load EGL, so this may be NULL. Ok, I was inspired by Gles2TextureToEglImageTranslator, which does the same. I guess we need to fix that one too. https://chromiumcodereview.appspot.com/11076009/diff/3001/content/common/gpu/... content/common/gpu/media/omx_video_decode_accelerator.cc:416: EGLSyncKHR egl_sync_obj = egl_create_sync_khr(egl_display_, On 2012/10/06 21:54:53, sheu wrote: > I'd be concerned about this leaking an EGLSyncKHR if tasks get cancelled > somehow. This can't happen now, since we let the class destructor destroy the > sync_thread_ (race conditions as piman points out), and all callbacks will > eventually be called. If you eventually make these callbacks though, you'll > have to wrap the EGLSyncKHR in some kind of ownership-tracked struct to make > sure we don't leak them. Yes, not happening now, but if we go with PostDelayedTask, we'll need refcounting. https://chromiumcodereview.appspot.com/11076009/diff/3001/content/common/gpu/... content/common/gpu/media/omx_video_decode_accelerator.cc:430: &OmxVideoDecodeAccelerator::WaitForPictureSync, weak_this_, On 2012/10/06 06:42:40, piman wrote: > You can't pass a weak pointer to another thread. It cannot be made thread-safe > (note: it'll assert in debug). > What you can do is put a pointer to the weak pointer into the closure, and call > a free function on the thread (as well as other params you need in there). That > function can post a task to the main thread where you can then dereference the > weak pointer before calling back into the class. I'll just use Unretained and weakptr back. This is always valid while there are any tasks on sync_thread. https://chromiumcodereview.appspot.com/11076009/diff/3001/content/common/gpu/... content/common/gpu/media/omx_video_decode_accelerator.cc:438: EGLint ret = egl_client_wait_sync_khr(egl_display_, egl_sync_obj, On 2012/10/06 06:42:40, piman wrote: > How do you know it's safe to access egl_display_ without a lock? What if this > gets deleted while this function is running? The thread is a member of this, so this can't be deleted with tasks running/pending. https://chromiumcodereview.appspot.com/11076009/diff/3001/content/common/gpu/... content/common/gpu/media/omx_video_decode_accelerator.cc:440: kSyncTimeoutNanosec); On 2012/10/06 06:42:40, piman wrote: > Why using a timeout at all? Well, the idea was to not block the whole thing forever in case something strange happened. Because otherwise we'd leak/wait forever. But maybe it's a bit too defensive. > A lock with a timeout is like no lock - imagine if you attach the debugger, or > if the thread isn't scheduled in time because the CPU was busy doing other > things, or whatever. You create races that are hard to repro/debug. > If we're worried about blocking the thread for too long, then we should post a > task to try again, not return success after the failure. Of course, but I thought it negligible, because then it's just that texture contents would possibly be overwritten. We didn't care about it until now and still don't care about it in other VDAs. On the other hand though I was worried of never returning the buffer to the decoder and leaking/hanging. But that would happen if the sync object somehow never got signaled. I wonder if it even makes sense to guard against it. Is it possible? Could we somehow drop the command stream's contents thus never signalling this? https://chromiumcodereview.appspot.com/11076009/diff/3001/content/common/gpu/... content/common/gpu/media/omx_video_decode_accelerator.cc:449: message_loop_->PostTask(FROM_HERE, base::Bind( On 2012/10/06 06:42:40, piman wrote: > Same here, accessing message_loop_ isn't safe. Instead, pass a message loop > proxy to the function. Passing weak pointer to sync_thread is wrong and you were right to point this out above, but here it is correct, we want to be passing a weak pointer back to the main thread (the one it's been created on). This will be valid under all circumstances (see my previous comment).
I like this version *much* better! https://codereview.chromium.org/11076009/diff/17001/content/common/gpu/media/... File content/common/gpu/media/omx_video_decode_accelerator.cc (right): https://codereview.chromium.org/11076009/diff/17001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:400: bool Create(EGLDisplay egl_display); All of the error conditions that can cause createsynckhr to fail are programming errors (not runtime-variable conditions) so you can just inline this into the ctor, and DCHECK that the resulting sync obj !=EGL_NO_SYNC_KHR. https://codereview.chromium.org/11076009/diff/17001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:426: egl_sync_obj_ = egl_create_sync_khr(egl_display_, EGL_SYNC_FENCE_KHR, 0); s/0/NULL/ https://codereview.chromium.org/11076009/diff/17001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:445: DCHECK(ret) << "EGL sync failed, continuing without syncing."; lolwat? In Debug, ain't nuthin' continuing after a DCHECK. In Release, this message is never printed. But, as above, the only error path for get_sync_attrib is programming error, so IMO you should DCHECK(ret) at l.439 and replace the rest of the method with return value == EGL_SIGNALED_KHR; (and also remove the verbiage about error from the method-level comment) https://codereview.chromium.org/11076009/diff/17001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:472: // It's possible for this task to get cancelled if the decoder is being s/get cancelled/never run/ https://codereview.chromium.org/11076009/diff/17001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:472: // It's possible for this task to get cancelled if the decoder is being s/decoder is destroyed/messageloop is stopped/ https://codereview.chromium.org/11076009/diff/17001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:480: &OmxVideoDecodeAccelerator::CheckPictureStatus, base::Unretained(this), s/base::Unretained(this)/weak_this_/ because what guarantees that (base::Unretained) |this| will point at a valid object 5ms from now??? https://codereview.chromium.org/11076009/diff/17001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:481: picture_buffer_id, base::Passed(egl_sync_obj.Pass())), replace egl_sync_obj.Pass() with &egl_sync_obj https://codereview.chromium.org/11076009/diff/17001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:486: // Synced successfully or getting sync status failed. Queue the buffer for drop "failed" clause
https://codereview.chromium.org/11076009/diff/17001/content/common/gpu/media/... File content/common/gpu/media/omx_video_decode_accelerator.cc (right): https://codereview.chromium.org/11076009/diff/17001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:400: bool Create(EGLDisplay egl_display); On 2012/10/12 17:20:05, Ami Fischman wrote: > All of the error conditions that can cause createsynckhr to fail are programming > errors (not runtime-variable conditions) so you can just inline this into the > ctor, and DCHECK that the resulting sync obj !=EGL_NO_SYNC_KHR. Not sure what you mean, egl_create_sync_khr may fail in runtime for driver-specific reasons... https://codereview.chromium.org/11076009/diff/17001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:445: DCHECK(ret) << "EGL sync failed, continuing without syncing."; On 2012/10/12 17:20:05, Ami Fischman wrote: > lolwat? > In Debug, ain't nuthin' continuing after a DCHECK. > In Release, this message is never printed. > > But, as above, the only error path for get_sync_attrib is programming error, so > IMO you should DCHECK(ret) at l.439 and replace the rest of the method with > return value == EGL_SIGNALED_KHR; > > (and also remove the verbiage about error from the method-level comment) lolled myself, not sure what I was thinking. I'm not sure if I want any of those calls to kill us though, we could just continue without syncing as we are doing now...
https://codereview.chromium.org/11076009/diff/17001/content/common/gpu/media/... File content/common/gpu/media/omx_video_decode_accelerator.cc (right): https://codereview.chromium.org/11076009/diff/17001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:400: bool Create(EGLDisplay egl_display); On 2012/10/12 17:50:50, posciak wrote: > On 2012/10/12 17:20:05, Ami Fischman wrote: > > All of the error conditions that can cause createsynckhr to fail are > programming > > errors (not runtime-variable conditions) so you can just inline this into the > > ctor, and DCHECK that the resulting sync obj !=EGL_NO_SYNC_KHR. > > Not sure what you mean, egl_create_sync_khr may fail in runtime for > driver-specific reasons... Where does the spec permit that? Do you mean s/reasons/bugs/? That reasoning could apply to any interaction with any other component, so isn't particularly convincing.
https://codereview.chromium.org/11076009/diff/17001/content/common/gpu/media/... File content/common/gpu/media/omx_video_decode_accelerator.cc (right): https://codereview.chromium.org/11076009/diff/17001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:400: bool Create(EGLDisplay egl_display); On 2012/10/12 17:57:28, Ami Fischman wrote: > On 2012/10/12 17:50:50, posciak wrote: > > On 2012/10/12 17:20:05, Ami Fischman wrote: > > > All of the error conditions that can cause createsynckhr to fail are > > programming > > > errors (not runtime-variable conditions) so you can just inline this into > the > > > ctor, and DCHECK that the resulting sync obj !=EGL_NO_SYNC_KHR. > > > > Not sure what you mean, egl_create_sync_khr may fail in runtime for > > driver-specific reasons... > > Where does the spec permit that? > Do you mean s/reasons/bugs/? That reasoning could apply to any interaction with > any other component, so isn't particularly convincing. Well, mostly things like out of memory or driver implementation-specific, runtime errors. I guess not good enough to guard against it. https://codereview.chromium.org/11076009/diff/17001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:426: egl_sync_obj_ = egl_create_sync_khr(egl_display_, EGL_SYNC_FENCE_KHR, 0); On 2012/10/12 17:20:05, Ami Fischman wrote: > s/0/NULL/ Done. https://codereview.chromium.org/11076009/diff/17001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:445: DCHECK(ret) << "EGL sync failed, continuing without syncing."; On 2012/10/12 17:50:50, posciak wrote: > On 2012/10/12 17:20:05, Ami Fischman wrote: > > lolwat? > > In Debug, ain't nuthin' continuing after a DCHECK. > > In Release, this message is never printed. > > > > But, as above, the only error path for get_sync_attrib is programming error, > so > > IMO you should DCHECK(ret) at l.439 and replace the rest of the method with > > return value == EGL_SIGNALED_KHR; > > > > (and also remove the verbiage about error from the method-level comment) > > lolled myself, not sure what I was thinking. > I'm not sure if I want any of those calls to kill us though, we could just > continue without syncing as we are doing now... Done. https://codereview.chromium.org/11076009/diff/17001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:472: // It's possible for this task to get cancelled if the decoder is being On 2012/10/12 17:20:05, Ami Fischman wrote: > s/get cancelled/never run/ Done. https://codereview.chromium.org/11076009/diff/17001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:472: // It's possible for this task to get cancelled if the decoder is being On 2012/10/12 17:20:05, Ami Fischman wrote: > s/decoder is destroyed/messageloop is stopped/ Done. https://codereview.chromium.org/11076009/diff/17001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:480: &OmxVideoDecodeAccelerator::CheckPictureStatus, base::Unretained(this), On 2012/10/12 17:20:05, Ami Fischman wrote: > s/base::Unretained(this)/weak_this_/ because what guarantees that > (base::Unretained) |this| will point at a valid object 5ms from now??? Another brain fart, must've still been in the sub-thread mindset when I wrote this. https://codereview.chromium.org/11076009/diff/17001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:481: picture_buffer_id, base::Passed(egl_sync_obj.Pass())), On 2012/10/12 17:20:05, Ami Fischman wrote: > replace > egl_sync_obj.Pass() > with > &egl_sync_obj Ok. FWIW got inspired by http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/extensions/e.... https://codereview.chromium.org/11076009/diff/17001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:486: // Synced successfully or getting sync status failed. Queue the buffer for On 2012/10/12 17:20:05, Ami Fischman wrote: > drop "failed" clause Done.
LGTM % nits https://codereview.chromium.org/11076009/diff/17001/content/common/gpu/media/... File content/common/gpu/media/omx_video_decode_accelerator.cc (right): https://codereview.chromium.org/11076009/diff/17001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:481: picture_buffer_id, base::Passed(egl_sync_obj.Pass())), On 2012/10/12 18:39:28, posciak wrote: > On 2012/10/12 17:20:05, Ami Fischman wrote: > > replace > > egl_sync_obj.Pass() > > with > > &egl_sync_obj > > Ok. FWIW got inspired by > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/extensions/e.... Huh; filed bug 155593 to eradicate that pattern. https://codereview.chromium.org/11076009/diff/22001/content/common/gpu/media/... File content/common/gpu/media/omx_video_decode_accelerator.cc (right): https://codereview.chromium.org/11076009/diff/22001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:399: // Check the status of the sync object. Return true if synced. Style-guide-sanctioned comment style would be: // Returns true if object is synced. which is probably not adding any information content beyond the signature of the function, so I'd just drop it. https://codereview.chromium.org/11076009/diff/22001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:414: << "Failed creating a sync object, will not sync."; msg is still bogus (not only will the process not sync, it's crashing!) I'd just drop the message entirely since the body of the assertion (which is emitted on crash) is informative. https://codereview.chromium.org/11076009/diff/22001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:418: if (egl_sync_obj_ != EGL_NO_SYNC_KHR) Not possible for this to be false anymore, so can drop the if. https://codereview.chromium.org/11076009/diff/22001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:428: if (value == EGL_UNSIGNALED_KHR) l.428-431 is equiv to return value == EGL_SIGNALED_KHR;
https://codereview.chromium.org/11076009/diff/22001/content/common/gpu/media/... File content/common/gpu/media/omx_video_decode_accelerator.cc (right): https://codereview.chromium.org/11076009/diff/22001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:393: class OmxVideoDecodeAccelerator::PictureSyncObject { nit: we usually don't define inner classes in the middle of defining the methods of the outer class. Mind moving this before the OmxVideoDecodeAccelerator constructor? https://codereview.chromium.org/11076009/diff/22001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:423: EGLint value; nit: = 0 https://codereview.chromium.org/11076009/diff/22001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:460: base::TimeDelta::FromMilliseconds(5)); nit: can you make this a constant in global scope (e.g. kSyncPollDelayMs)
(LGTM with nits addressed)
https://codereview.chromium.org/11076009/diff/22001/content/common/gpu/media/... File content/common/gpu/media/omx_video_decode_accelerator.cc (right): https://codereview.chromium.org/11076009/diff/22001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:423: EGLint value; On 2012/10/12 19:37:10, piman wrote: > nit: = 0 OOC: Why?
On Fri, Oct 12, 2012 at 12:43 PM, <fischman@chromium.org> wrote: > > https://codereview.chromium.**org/11076009/diff/22001/** > content/common/gpu/media/omx_**video_decode_accelerator.cc<https://codereview.chromium.org/11076009/diff/22001/content/common/gpu/media/omx_video_decode_accelerator.cc> > File content/common/gpu/media/omx_**video_decode_accelerator.cc (right): > > https://codereview.chromium.**org/11076009/diff/22001/** > content/common/gpu/media/omx_**video_decode_accelerator.cc#**newcode423<https://codereview.chromium.org/11076009/diff/22001/content/common/gpu/media/omx_video_decode_accelerator.cc#newcode423> > content/common/gpu/media/omx_**video_decode_accelerator.cc:**423: EGLint > value; > On 2012/10/12 19:37:10, piman wrote: > >> nit: = 0 >> > > OOC: Why? > Initialize variables? If there is an error, value won't be set. You can argue that there shouldn't be an error, but that's the same with all Chrome code where we initialize variables anyway. It prevents a surprise the day someone changes code that introduces an error case. > > https://codereview.chromium.**org/11076009/<https://codereview.chromium.org/1... >
> nit: = 0 >> >> OOC: Why? > > Initialize variables? > If there is an error, value won't be set. > You can argue that there shouldn't be an error, but that's the same with > all Chrome code where we initialize variables anyway. It prevents a > surprise the day someone changes code that introduces an error case. > The flip side is that vacuous initializations prevent code analysis tools from detecting legit uninit'd accesses. (obvs., this conversation is bigger than this CL, and neither of us thinks it's blocking here)
On Fri, Oct 12, 2012 at 1:03 PM, Ami Fischman <fischman@chromium.org> wrote: > > nit: = 0 >>> >>> OOC: Why? >> >> Initialize variables? >> If there is an error, value won't be set. >> You can argue that there shouldn't be an error, but that's the same with >> all Chrome code where we initialize variables anyway. It prevents a >> surprise the day someone changes code that introduces an error case. >> > > The flip side is that vacuous initializations prevent code analysis tools > from detecting legit uninit'd accesses. > (obvs., this conversation is bigger than this CL, and neither of us thinks > it's blocking here) > -> chromium-dev if you want to change the status quo.
On Fri, Oct 12, 2012 at 1:05 PM, Antoine Labour <piman@chromium.org> wrote: > -> chromium-dev if you want to change the status quo. > Haha - I was going to write the same thing. I guess we have different opinions of the status quo. Will leave this reviewlog thread alone now.
On Fri, Oct 12, 2012 at 1:08 PM, Ami Fischman <fischman@chromium.org> wrote: > On Fri, Oct 12, 2012 at 1:05 PM, Antoine Labour <piman@chromium.org>wrote: > >> -> chromium-dev if you want to change the status quo. >> > > Haha - I was going to write the same thing. I guess we have different > opinions of the status quo. > Will leave this reviewlog thread alone now. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Local_...
https://codereview.chromium.org/11076009/diff/22001/content/common/gpu/media/... File content/common/gpu/media/omx_video_decode_accelerator.cc (right): https://codereview.chromium.org/11076009/diff/22001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:393: class OmxVideoDecodeAccelerator::PictureSyncObject { On 2012/10/12 19:37:10, piman wrote: > nit: we usually don't define inner classes in the middle of defining the methods > of the outer class. Mind moving this before the OmxVideoDecodeAccelerator > constructor? Ok, if that's the convention. (The idea was to have it near the only use site.) https://codereview.chromium.org/11076009/diff/22001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:399: // Check the status of the sync object. Return true if synced. On 2012/10/12 19:34:46, Ami Fischman wrote: > Style-guide-sanctioned comment style would be: > // Returns true if object is synced. > which is probably not adding any information content beyond the signature of the > function, so I'd just drop it. Done. https://codereview.chromium.org/11076009/diff/22001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:414: << "Failed creating a sync object, will not sync."; On 2012/10/12 19:34:46, Ami Fischman wrote: > msg is still bogus (not only will the process not sync, it's crashing!) > I'd just drop the message entirely since the body of the assertion (which is > emitted on crash) is informative. Heh. Somehow my brain won't acknowledge that this an assert. https://codereview.chromium.org/11076009/diff/22001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:418: if (egl_sync_obj_ != EGL_NO_SYNC_KHR) On 2012/10/12 19:34:46, Ami Fischman wrote: > Not possible for this to be false anymore, so can drop the if. Done. https://codereview.chromium.org/11076009/diff/22001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:423: EGLint value; On 2012/10/12 19:37:10, piman wrote: > nit: = 0 Are you worried the call would exit early before overwriting it? But then I think it'd be safe to assume ret would be != EGL_TRUE. https://codereview.chromium.org/11076009/diff/22001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:428: if (value == EGL_UNSIGNALED_KHR) On 2012/10/12 19:34:46, Ami Fischman wrote: > l.428-431 is equiv to > return value == EGL_SIGNALED_KHR; Done. https://codereview.chromium.org/11076009/diff/22001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:460: base::TimeDelta::FromMilliseconds(5)); On 2012/10/12 19:37:10, piman wrote: > nit: can you make this a constant in global scope (e.g. kSyncPollDelayMs) Done.
https://codereview.chromium.org/11076009/diff/22001/content/common/gpu/media/... File content/common/gpu/media/omx_video_decode_accelerator.cc (right): https://codereview.chromium.org/11076009/diff/22001/content/common/gpu/media/... content/common/gpu/media/omx_video_decode_accelerator.cc:423: EGLint value; On 2012/10/12 20:33:08, posciak wrote: > On 2012/10/12 19:37:10, piman wrote: > > nit: = 0 > > Are you worried the call would exit early before overwriting it? But then I > think it'd be safe to assume ret would be != EGL_TRUE. Oh, but that's only only in debug builds. I really need more caffeine today.
All fixed now hopefully.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/posciak@chromium.org/11076009/26004
Change committed as 161759 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
