Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22590009:
EVDA: Add support for dynamic resolution change and MSE players. (Closed)
|
Created:
7 years, 4 months ago by Pawel Osciak Modified:
7 years, 4 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, apatrick_chromium, feature-media-reviews_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionEVDA: Add support for dynamic resolution change and MSE players.
This adds a resolution switch sequence, initiated by the driver sending us
a V4L2_EVENT_RESOLUTION_CHANGED event. After receiving it, we finish
processing existing output buffers and replace them with a new set for
the new format, provided by MFC. Input buffers are kept intact in order
to be able to continue decoding them after the switch.
Also, separately, perform a dummy STREAMOFF-STREAMON cycle on Flush().
This fixes freezes in MSE player between stream chunks, as the player
triggers a Flush() at end of chunk, but no Reset(). MFC requires reset
to continue after flush.
BUG=177422, 238488
TEST=regular playback, unittests, resolution switch streams, MSE tests
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216969
Patch Set 1 #
Total comments: 21
Patch Set 2 : #
Total comments: 8
Patch Set 3 : #
Total comments: 1
Patch Set 4 : #
Total comments: 29
Patch Set 5 : #
Total comments: 6
Messages
Total messages: 22 (0 generated)
PTAL!
Initial bits. https://chromiumcodereview.appspot.com/22590009/diff/1/content/common/gpu/med... File content/common/gpu/media/exynos_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/22590009/diff/1/content/common/gpu/med... content/common/gpu/media/exynos_video_decode_accelerator.cc:861: if (ioctl(mfc_fd_, VIDIOC_G_FMT, format) != 0) { I should have probably wrapped this in HANDLE_EINTR the first time. https://chromiumcodereview.appspot.com/22590009/diff/1/content/common/gpu/med... content/common/gpu/media/exynos_video_decode_accelerator.cc:886: DVLOG(3) << "New resolution: " << frame_buffer_size_.ToString(); I got in the habit of "CreateBuffersForFormat(): BLAH BLAH BLAH" for comments. Keep it up here? yay linebreaks https://chromiumcodereview.appspot.com/22590009/diff/1/content/common/gpu/med... content/common/gpu/media/exynos_video_decode_accelerator.cc:933: DVLOG(3) << "DecodeBufferInitial(): running one-time initialization"; It's not one-time anymore. https://chromiumcodereview.appspot.com/22590009/diff/1/content/common/gpu/med... content/common/gpu/media/exynos_video_decode_accelerator.cc:1191: DCHECK_NE(decoder_state_, kUninitialized); DCHECK_EQ(decoder_state_, kDecoding) https://chromiumcodereview.appspot.com/22590009/diff/1/content/common/gpu/med... content/common/gpu/media/exynos_video_decode_accelerator.cc:1196: if (!mfc_output_gsc_input_queue_.empty() || Do we need to wait for mfc_output_buffer_queued_count_ to drain too? Does resolution change force DPB flush? If we assume that resolution change is only tagged on the last MFC output buffer out of the device, it would be nice to DCHECK the count. https://chromiumcodereview.appspot.com/22590009/diff/1/content/common/gpu/med... content/common/gpu/media/exynos_video_decode_accelerator.cc:1216: void ExynosVideoDecodeAccelerator::ResolutionChangeDestroyBuffers() { We should run this on the decoder_thread_, since the decoder_thread_ is still outstanding (and to avoid icky touching of state from two threads). DestroyGscOutputBuffers() should be safe to call from this thread, since eglDestroyImageKHR and eglDestroySyncKHR should be safe from any thread, even if there is no current EGLContext. That would actually allow us to do all three of the ResolutionChange tasks at once. Sweet! https://chromiumcodereview.appspot.com/22590009/diff/1/content/common/gpu/med... content/common/gpu/media/exynos_video_decode_accelerator.cc:1312: DCHECK_NE(decoder_state_, kUninitialized); this really should just be DCHECK_EQ(decoder_state_, kDecoding) https://chromiumcodereview.appspot.com/22590009/diff/1/content/common/gpu/med... content/common/gpu/media/exynos_video_decode_accelerator.cc:1796: void ExynosVideoDecodeAccelerator::ResetTask() { Drain the V4L2 event queue and reset resolution_change_pending_ https://chromiumcodereview.appspot.com/22590009/diff/1/content/common/gpu/med... content/common/gpu/media/exynos_video_decode_accelerator.cc:2396: DVLOG(1) << "Dismissing PictureBuffer id=" << output_record.picture_id; (same as above) "DestroyGscOutputBuffers(): XXX " ? https://chromiumcodereview.appspot.com/22590009/diff/1/content/common/gpu/med... File content/common/gpu/media/exynos_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/22590009/diff/1/content/common/gpu/med... content/common/gpu/media/exynos_video_decode_accelerator.h:259: bool GetFormatInfo(struct v4l2_format* format, bool* again); Move this closer to DecodeBufferInitial methinks. https://chromiumcodereview.appspot.com/22590009/diff/1/content/common/gpu/med... content/common/gpu/media/exynos_video_decode_accelerator.h:260: void ResolutionChangeDestroyBuffers(); It would be nice to move this to another section just for child thread tasks. (edit): actually, removing it altogether would be nice. (see .cc) https://chromiumcodereview.appspot.com/22590009/diff/1/content/common/gpu/med... content/common/gpu/media/exynos_video_decode_accelerator.h:289: bool CreateBuffersForFormat(const struct v4l2_format& format); Doesn't require a forward decl? hmm
https://chromiumcodereview.appspot.com/22590009/diff/1/content/common/gpu/med... File content/common/gpu/media/exynos_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/22590009/diff/1/content/common/gpu/med... content/common/gpu/media/exynos_video_decode_accelerator.cc:861: if (ioctl(mfc_fd_, VIDIOC_G_FMT, format) != 0) { On 2013/08/09 09:23:16, sheu wrote: > I should have probably wrapped this in HANDLE_EINTR the first time. Done. https://chromiumcodereview.appspot.com/22590009/diff/1/content/common/gpu/med... content/common/gpu/media/exynos_video_decode_accelerator.cc:886: DVLOG(3) << "New resolution: " << frame_buffer_size_.ToString(); On 2013/08/09 09:23:16, sheu wrote: > I got in the habit of "CreateBuffersForFormat(): BLAH BLAH BLAH" for comments. > Keep it up here? > > yay linebreaks Done. https://chromiumcodereview.appspot.com/22590009/diff/1/content/common/gpu/med... content/common/gpu/media/exynos_video_decode_accelerator.cc:933: DVLOG(3) << "DecodeBufferInitial(): running one-time initialization"; On 2013/08/09 09:23:16, sheu wrote: > It's not one-time anymore. Done. https://chromiumcodereview.appspot.com/22590009/diff/1/content/common/gpu/med... content/common/gpu/media/exynos_video_decode_accelerator.cc:1191: DCHECK_NE(decoder_state_, kUninitialized); On 2013/08/09 09:23:16, sheu wrote: > DCHECK_EQ(decoder_state_, kDecoding) Done. https://chromiumcodereview.appspot.com/22590009/diff/1/content/common/gpu/med... content/common/gpu/media/exynos_video_decode_accelerator.cc:1196: if (!mfc_output_gsc_input_queue_.empty() || On 2013/08/09 09:23:16, sheu wrote: > Do we need to wait for mfc_output_buffer_queued_count_ to drain too? Does > resolution change force DPB flush? > > If we assume that resolution change is only tagged on the last MFC output buffer > out of the device, it would be nice to DCHECK the count. The event will happen after MFC internal queues are drained and are ready to DQBUF. We do that in DequeueMfc() immediately after DequeueEvents() in ServiceDeviceTask(). Good idea to dcheck though. https://chromiumcodereview.appspot.com/22590009/diff/1/content/common/gpu/med... content/common/gpu/media/exynos_video_decode_accelerator.cc:1216: void ExynosVideoDecodeAccelerator::ResolutionChangeDestroyBuffers() { On 2013/08/09 09:23:16, sheu wrote: > We should run this on the decoder_thread_, since the decoder_thread_ is still > outstanding (and to avoid icky touching of state from two threads). > DestroyGscOutputBuffers() should be safe to call from this thread, since > eglDestroyImageKHR and eglDestroySyncKHR should be safe from any thread, even if > there is no current EGLContext. > > That would actually allow us to do all three of the ResolutionChange tasks at > once. Sweet! As per offline chat, we'll do this in a separate CL in order not to have to revert this if anything explodes after move to decoder thread, and to make it easier to isolate in case of any problems after both CLs. https://chromiumcodereview.appspot.com/22590009/diff/1/content/common/gpu/med... content/common/gpu/media/exynos_video_decode_accelerator.cc:1312: DCHECK_NE(decoder_state_, kUninitialized); On 2013/08/09 09:23:16, sheu wrote: > this really should just be DCHECK_EQ(decoder_state_, kDecoding) Done. https://chromiumcodereview.appspot.com/22590009/diff/1/content/common/gpu/med... content/common/gpu/media/exynos_video_decode_accelerator.cc:1796: void ExynosVideoDecodeAccelerator::ResetTask() { On 2013/08/09 09:23:16, sheu wrote: > Drain the V4L2 event queue and reset resolution_change_pending_ Done. https://chromiumcodereview.appspot.com/22590009/diff/1/content/common/gpu/med... content/common/gpu/media/exynos_video_decode_accelerator.cc:2396: DVLOG(1) << "Dismissing PictureBuffer id=" << output_record.picture_id; On 2013/08/09 09:23:16, sheu wrote: > (same as above) "DestroyGscOutputBuffers(): XXX " ? Done.
Two basically nitpicky bits. Cheers. LGTM otherwise https://chromiumcodereview.appspot.com/22590009/diff/7001/content/common/gpu/... File content/common/gpu/media/exynos_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/22590009/diff/7001/content/common/gpu/... content/common/gpu/media/exynos_video_decode_accelerator.h:260: void ResolutionChangeDestroyBuffers(); If you're not removing this just yet it would be nice to call it XXXOnChildThread or something similarly obvious https://chromiumcodereview.appspot.com/22590009/diff/7001/content/common/gpu/... content/common/gpu/media/exynos_video_decode_accelerator.h:289: bool CreateBuffersForFormat(const struct v4l2_format& format); Also: I kept the function definitions and declarations in the same order. Style guide seems to have a preference for it, though you may not personally be as OCD.
racy. https://chromiumcodereview.appspot.com/22590009/diff/7001/content/common/gpu/... File content/common/gpu/media/exynos_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/22590009/diff/7001/content/common/gpu/... content/common/gpu/media/exynos_video_decode_accelerator.cc:1800: void ExynosVideoDecodeAccelerator::ResetTask() { OK, this could be a little bad. What if we: 1. Decoder thread starts resolution change, posts ResolutionChangeDestroyBuffers() to the child thread 2. Child thread gets a Reset() call, posts ResetTask() to the decoder thread 3. Child thread runs ResolutionChangeDestroyBuffers(), posts a FinishResolutionChangeTask() to decoder thread 4. Decoder thread runs ResetTask(), state is now kResetting, posts a ResetDoneTask() to decoder thread 5. Decoder thread runs FinishResolutionChangeTask(), asks for buffers with PPB after (4), decoder_state_ is no longer kChangingResolution (and it will be kAfterReset after ResetDoneTask(), so the ResumeAfterResolutionChangeTask() never gets run at the end of APB. There's probably other variations here too. https://chromiumcodereview.appspot.com/22590009/diff/7001/content/common/gpu/... content/common/gpu/media/exynos_video_decode_accelerator.cc:1810: // We stop streaming, but we _don't_ destroy our buffers. This comment is a little confusing now, with the 'false' parameter below it seems to imply the parameter means not to destroy the buffers. Maybe something like // We stop streaming and clear buffer tracking info (not preserving MFC inputs). StopDevicePoll() unconditionally does _not_ destroy buffers, however.
https://chromiumcodereview.appspot.com/22590009/diff/7001/content/common/gpu/... File content/common/gpu/media/exynos_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/22590009/diff/7001/content/common/gpu/... content/common/gpu/media/exynos_video_decode_accelerator.cc:1800: void ExynosVideoDecodeAccelerator::ResetTask() { On 2013/08/09 11:04:08, sheu wrote: > OK, this could be a little bad. > > What if we: > > 1. Decoder thread starts resolution change, posts > ResolutionChangeDestroyBuffers() to the child thread > 2. Child thread gets a Reset() call, posts ResetTask() to the decoder thread > 3. Child thread runs ResolutionChangeDestroyBuffers(), posts a > FinishResolutionChangeTask() to decoder thread > 4. Decoder thread runs ResetTask(), state is now kResetting, posts a > ResetDoneTask() to decoder thread > 5. Decoder thread runs FinishResolutionChangeTask(), asks for buffers with PPB > > after (4), decoder_state_ is no longer kChangingResolution (and it will be > kAfterReset after ResetDoneTask(), so the ResumeAfterResolutionChangeTask() > never gets run at the end of APB. > > There's probably other variations here too. Added fix as per convo. https://chromiumcodereview.appspot.com/22590009/diff/7001/content/common/gpu/... content/common/gpu/media/exynos_video_decode_accelerator.cc:1810: // We stop streaming, but we _don't_ destroy our buffers. On 2013/08/09 11:04:08, sheu wrote: > This comment is a little confusing now, with the 'false' parameter below it > seems to imply the parameter means not to destroy the buffers. > > Maybe something like > > // We stop streaming and clear buffer tracking info (not preserving MFC inputs). > StopDevicePoll() unconditionally does _not_ destroy buffers, however. Done. https://chromiumcodereview.appspot.com/22590009/diff/7001/content/common/gpu/... File content/common/gpu/media/exynos_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/22590009/diff/7001/content/common/gpu/... content/common/gpu/media/exynos_video_decode_accelerator.h:260: void ResolutionChangeDestroyBuffers(); On 2013/08/09 10:41:49, sheu wrote: > If you're not removing this just yet it would be nice to call it > XXXOnChildThread or something similarly obvious Done. https://chromiumcodereview.appspot.com/22590009/diff/7001/content/common/gpu/... content/common/gpu/media/exynos_video_decode_accelerator.h:289: bool CreateBuffersForFormat(const struct v4l2_format& format); On 2013/08/09 10:41:49, sheu wrote: > Also: I kept the function definitions and declarations in the same order. Style > guide seems to have a preference for it, though you may not personally be as > OCD. Done.
Looks good for this particular race: Reset()/resolution change.
Reverting the previously-suggested DCHECK, which is wrong after all. https://chromiumcodereview.appspot.com/22590009/diff/15001/content/common/gpu... File content/common/gpu/media/exynos_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/22590009/diff/15001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.cc:1910: DCHECK_EQ(mfc_output_buffer_queued_count_, 0); So John fooled me, this is not required and actually wrong. We have: if (event_pending) DequeueEvents(); DequeueMfc(); DequeueGsc(); EnqueueMfc(); EnqueueGsc(); ... StartResolutionChangeIfNeeded(); in uninterrupted sequence. First DequeueEvents() will signal change if all outputs before change are done and are ready to be dequeued. So the subsequent DequeueMfc() will dequeue everything we are interested in. But then we immediately EnqueueMfc(), so we can potentially enqueue some remaining empty buffers back onto output, which we will never use anyway, but this check will then fail, without any good reason. So yeah, but no.
https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... File content/common/gpu/media/exynos_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.cc:389: // Subscribe for the resolution change event. s/for/to/ https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.cc:1207: } else DLOG(FATAL) since this is the only type we subscribed to? https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.cc:1659: // MSE player however triggers a Flush() on chunk end, but never Reset(). One Can you substantiate this assertion: "MSE player however triggers a Flush() on chunk end, but never Reset()" ? This is not how I understand things to work, and is likely a performance bug if it is in fact happening. (Flush should only be necessary when followed by Reset; if no Reset is coming there's no need for Flush) https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.cc:1692: // before the MFC input pipe is already stopped if we are changing resolution. s/before/because/ ? https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.cc:1693: // We will come back here after we are done with the resolution change. Did you mean to return; from the if block? https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.cc:1956: DVLOG(3) << "Couldn't get format information after resolution change"; Isn't this a fatal error for the decoder? (what's going to re-post this task?) https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.cc:1961: DVLOG(3) << "Couldn't reallocate buffers after resolution change"; ditto https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.cc:2028: bool event_pending = (mfc_pollfd != -1 && Is it important to do this, vs. unconditionally always trying to read events and exiting after 0 loop iterations? https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... File content/common/gpu/media/exynos_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.h:210: void ServiceDeviceTask(bool event_pending); Doco what |event_pending| means. Since only MFC is inspected in DequeueEvents, I suspect both this param and that method want to have "mfc" in the name in some way. https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.h:253: bool StopDevicePoll(bool keep_mfc_input); s/input/input_state/ ? https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.h:259: void ResChangeDestroyBuffersOnChildThread(); s/Res/Resolution/ https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.h:259: void ResChangeDestroyBuffersOnChildThread(); This "OnChildThread" is out of whack with the rest of the layout of this class (methods divided by which thread they run on). Can you improve this? https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.h:288: bool GetFormatInfo(struct v4l2_format* format, bool* again); doco https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.h:297: void DestroyMfcOutputBuffers(); These methods are annotated as running on decoder_thread_ unless that hasn't started yet in which case child_thread_, but they all DCHECK that they're running on the child thread. Should they move to a child-thread section? https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.h:350: bool reset_pending_; var name should reflect fact of resolution change
No CL yet. https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... File content/common/gpu/media/exynos_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.cc:1659: // MSE player however triggers a Flush() on chunk end, but never Reset(). One On 2013/08/09 16:55:41, Ami Fischman wrote: > Can you substantiate this assertion: "MSE player however triggers a Flush() on > chunk end, but never Reset()" ? > This is not how I understand things to work, and is likely a performance bug if > it is in fact happening. > > (Flush should only be necessary when followed by Reset; if no Reset is coming > there's no need for Flush) I had time to look at this only briefly. EVDA was getting a Flush() at the end of the chunk, but not Reset(). GVD gets an empty buffer so sends the flush. That's all I know for now. I intend to start a separate thread about this after more investigation, but didn't have enough time yet. https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.cc:2028: bool event_pending = (mfc_pollfd != -1 && On 2013/08/09 16:55:41, Ami Fischman wrote: > Is it important to do this, vs. unconditionally always trying to read events and > exiting after 0 loop iterations? Always better not to spam ioctls every frame, and this happens extremely rarely also. https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... File content/common/gpu/media/exynos_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.h:259: void ResChangeDestroyBuffersOnChildThread(); On 2013/08/09 16:55:41, Ami Fischman wrote: > This "OnChildThread" is out of whack with the rest of the layout of this class > (methods divided by which thread they run on). Can you improve this? I agree. I added it after John's comment on PS1, who preferred it this way. But I'd also prefer it without it. So you guys fight it out before I get my desktop back ;)
On Aug 9, 2013 10:40 PM, <posciak@chromium.org> wrote: > > No CL yet. > > > > https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... > File content/common/gpu/media/exynos_video_decode_accelerator.cc > (right): > > https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... > content/common/gpu/media/exynos_video_decode_accelerator.cc:1659: // MSE > player however triggers a Flush() on chunk end, but never Reset(). One > On 2013/08/09 16:55:41, Ami Fischman wrote: >> >> Can you substantiate this assertion: "MSE player however triggers a > > Flush() on >> >> chunk end, but never Reset()" ? >> This is not how I understand things to work, and is likely a > > performance bug if >> >> it is in fact happening. > > >> (Flush should only be necessary when followed by Reset; if no Reset is > > coming >> >> there's no need for Flush) > > > I had time to look at this only briefly. EVDA was getting a Flush() at > the end of the chunk, but not Reset(). GVD gets an empty buffer so sends > the flush. That's all I know for now. I intend to start a separate > thread about this after more investigation, but didn't have enough time > yet. Well, I think it's wrong, so I don't want you landing the comment as is ;) (either figure out what's going on during CR or todo+crbug to figure it out shortly after landing) > > > https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... > content/common/gpu/media/exynos_video_decode_accelerator.cc:2028: bool > event_pending = (mfc_pollfd != -1 && > On 2013/08/09 16:55:41, Ami Fischman wrote: >> >> Is it important to do this, vs. unconditionally always trying to read > > events and >> >> exiting after 0 loop iterations? > > > Always better not to spam ioctls every frame, and this happens extremely > rarely also. > > > https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... > File content/common/gpu/media/exynos_video_decode_accelerator.h (right): > > https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... > content/common/gpu/media/exynos_video_decode_accelerator.h:259: void > ResChangeDestroyBuffersOnChildThread(); > On 2013/08/09 16:55:41, Ami Fischman wrote: >> >> This "OnChildThread" is out of whack with the rest of the layout of > > this class >> >> (methods divided by which thread they run on). Can you improve this? > > > I agree. I added it after John's comment on PS1, who preferred it this > way. But I'd also prefer it without it. So you guys fight it out before > I get my desktop back ;) I'm not saying the method shouldn't exist. I'm saying it should be located in the right section of the class layout. > > https://chromiumcodereview.appspot.com/22590009/
This time with a CL. https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... File content/common/gpu/media/exynos_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.cc:389: // Subscribe for the resolution change event. On 2013/08/09 16:55:41, Ami Fischman wrote: > s/for/to/ Done. https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.cc:1207: } On 2013/08/09 16:55:41, Ami Fischman wrote: > else DLOG(FATAL) since this is the only type we subscribed to? Done. https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.cc:1692: // before the MFC input pipe is already stopped if we are changing resolution. On 2013/08/09 16:55:41, Ami Fischman wrote: > s/before/because/ ? Done. https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.cc:1693: // We will come back here after we are done with the resolution change. On 2013/08/09 16:55:41, Ami Fischman wrote: > Did you mean to return; from the if block? Yes, it was there, as braces prove, and I have no idea where it went. Good thing you noticed. https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.cc:1956: DVLOG(3) << "Couldn't get format information after resolution change"; On 2013/08/09 16:55:41, Ami Fischman wrote: > Isn't this a fatal error for the decoder? > (what's going to re-post this task?) Yeah, good point, I should make it a platform error. https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.cc:1961: DVLOG(3) << "Couldn't reallocate buffers after resolution change"; On 2013/08/09 16:55:41, Ami Fischman wrote: > ditto Done. https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... File content/common/gpu/media/exynos_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.h:210: void ServiceDeviceTask(bool event_pending); On 2013/08/09 16:55:41, Ami Fischman wrote: > Doco what |event_pending| means. > Since only MFC is inspected in DequeueEvents, I suspect both this param and that > method want to have "mfc" in the name in some way. Done. https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.h:253: bool StopDevicePoll(bool keep_mfc_input); On 2013/08/09 16:55:41, Ami Fischman wrote: > s/input/input_state/ ? Done. https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.h:288: bool GetFormatInfo(struct v4l2_format* format, bool* again); On 2013/08/09 16:55:41, Ami Fischman wrote: > doco Done. https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.h:297: void DestroyMfcOutputBuffers(); On 2013/08/09 16:55:41, Ami Fischman wrote: > These methods are annotated as running on decoder_thread_ unless that hasn't > started yet in which case child_thread_, but they all DCHECK that they're > running on the child thread. Should they move to a child-thread section? Done. https://chromiumcodereview.appspot.com/22590009/diff/20001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.h:350: bool reset_pending_; On 2013/08/09 16:55:41, Ami Fischman wrote: > var name should reflect fact of resolution change Done.
https://chromiumcodereview.appspot.com/22590009/diff/27001/content/common/gpu... File content/common/gpu/media/exynos_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/22590009/diff/27001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.h:261: void FinishResolutionChange(); I noted before that I'd like to see these moved to a child-thread section. Now that you have one an put it there, I don't think it should be a problem.
https://chromiumcodereview.appspot.com/22590009/diff/27001/content/common/gpu... File content/common/gpu/media/exynos_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/22590009/diff/27001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.h:261: void FinishResolutionChange(); On 2013/08/12 03:06:17, sheu wrote: > I noted before that I'd like to see these moved to a child-thread section. Now > that you have one an put it there, I don't think it should be a problem. These three are run on decoder thread. Only ResolutionChangeDestroyBuffers() is run on child and I moved it to that section...
LGTM % nit, and added a comment on 270039 to hopefully move that conversation forward. But I don't think that needs to hold up this CL. https://chromiumcodereview.appspot.com/22590009/diff/27001/content/common/gpu... File content/common/gpu/media/exynos_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/22590009/diff/27001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.cc:1197: DVLOG(3) << "DequeueMfcEvents()"; Is there some reason you (and your kind, who enjoy these sort of tracer DVLOGs, apparently) use explicit strings instead of __FUNCTION__ / __func__ ?
https://chromiumcodereview.appspot.com/22590009/diff/27001/content/common/gpu... File content/common/gpu/media/exynos_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/22590009/diff/27001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.cc:1197: DVLOG(3) << "DequeueMfcEvents()"; On 2013/08/12 03:46:19, Ami Fischman wrote: > Is there some reason you (and your kind, who enjoy these sort of tracer DVLOGs, > apparently) use explicit strings instead of __FUNCTION__ / __func__ ? I do enjoy those traces and they help a lot when debugging, and I'm surprised you apparently seem to dislike them :) I agree that using macros is better and I debated using them, but I didn't want to break this class' conventions, which is to spell out the names. In any case, not sure if you are asking me to convert this class to using __FUNCTION__ or even __PRETTY_FUNCTION__ in this CL, or just making a general remark? :)
No need to change style in this CL. On Sun, Aug 11, 2013 at 9:07 PM, <posciak@chromium.org> wrote: > > https://chromiumcodereview.**appspot.com/22590009/diff/** > 27001/content/common/gpu/**media/exynos_video_decode_**accelerator.cc<https://chromiumcodereview.appspot.com/22590009/diff/27001/content/common/gpu/media/exynos_video_decode_accelerator.cc> > File content/common/gpu/media/**exynos_video_decode_**accelerator.cc > (right): > > https://chromiumcodereview.**appspot.com/22590009/diff/** > 27001/content/common/gpu/**media/exynos_video_decode_** > accelerator.cc#newcode1197<https://chromiumcodereview.appspot.com/22590009/diff/27001/content/common/gpu/media/exynos_video_decode_accelerator.cc#newcode1197> > content/common/gpu/media/**exynos_video_decode_**accelerator.cc:1197: > DVLOG(3) << "DequeueMfcEvents()"; > On 2013/08/12 03:46:19, Ami Fischman wrote: > >> Is there some reason you (and your kind, who enjoy these sort of >> > tracer DVLOGs, > >> apparently) use explicit strings instead of __FUNCTION__ / __func__ ? >> > > I do enjoy those traces and they help a lot when debugging, and I'm > surprised you apparently seem to dislike them :) I agree that using > macros is better and I debated using them, but I didn't want to break > this class' conventions, which is to spell out the names. > > In any case, not sure if you are asking me to convert this class to > using __FUNCTION__ or even __PRETTY_FUNCTION__ in this CL, or just > making a general remark? :) > > https://chromiumcodereview.**appspot.com/22590009/<https://chromiumcodereview... >
Thanks. I'll give John a chance to LGTM again before I hit commit. On 2013/08/12 04:12:57, Ami Fischman wrote: > No need to change style in this CL. > > > On Sun, Aug 11, 2013 at 9:07 PM, <mailto:posciak@chromium.org> wrote: > > > > > https://chromiumcodereview.**appspot.com/22590009/diff/** > > > 27001/content/common/gpu/**media/exynos_video_decode_**accelerator.cc<https://chromiumcodereview.appspot.com/22590009/diff/27001/content/common/gpu/media/exynos_video_decode_accelerator.cc> > > File content/common/gpu/media/**exynos_video_decode_**accelerator.cc > > (right): > > > > https://chromiumcodereview.**appspot.com/22590009/diff/** > > 27001/content/common/gpu/**media/exynos_video_decode_** > > > accelerator.cc#newcode1197<https://chromiumcodereview.appspot.com/22590009/diff/27001/content/common/gpu/media/exynos_video_decode_accelerator.cc#newcode1197> > > content/common/gpu/media/**exynos_video_decode_**accelerator.cc:1197: > > DVLOG(3) << "DequeueMfcEvents()"; > > On 2013/08/12 03:46:19, Ami Fischman wrote: > > > >> Is there some reason you (and your kind, who enjoy these sort of > >> > > tracer DVLOGs, > > > >> apparently) use explicit strings instead of __FUNCTION__ / __func__ ? > >> > > > > I do enjoy those traces and they help a lot when debugging, and I'm > > surprised you apparently seem to dislike them :) I agree that using > > macros is better and I debated using them, but I didn't want to break > > this class' conventions, which is to spell out the names. > > > > In any case, not sure if you are asking me to convert this class to > > using __FUNCTION__ or even __PRETTY_FUNCTION__ in this CL, or just > > making a general remark? :) > > > > > https://chromiumcodereview.**appspot.com/22590009/%3Chttps://chromiumcoderevi...> > >
lgtm https://chromiumcodereview.appspot.com/22590009/diff/27001/content/common/gpu... File content/common/gpu/media/exynos_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/22590009/diff/27001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.cc:1197: DVLOG(3) << "DequeueMfcEvents()"; On 2013/08/12 04:07:45, posciak wrote: > On 2013/08/12 03:46:19, Ami Fischman wrote: > > Is there some reason you (and your kind, who enjoy these sort of tracer > DVLOGs, > > apparently) use explicit strings instead of __FUNCTION__ / __func__ ? > > I do enjoy those traces and they help a lot when debugging, and I'm surprised > you apparently seem to dislike them :) I agree that using macros is better and I > debated using them, but I didn't want to break this class' conventions, which is > to spell out the names. > > In any case, not sure if you are asking me to convert this class to using > __FUNCTION__ or even __PRETTY_FUNCTION__ in this CL, or just making a general > remark? :) My fault. I just started doing it this way, and haven't stopped. I don't have any particular objection to switching if it feels necessary. https://chromiumcodereview.appspot.com/22590009/diff/27001/content/common/gpu... File content/common/gpu/media/exynos_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/22590009/diff/27001/content/common/gpu... content/common/gpu/media/exynos_video_decode_accelerator.h:261: void FinishResolutionChange(); On 2013/08/12 03:19:10, posciak wrote: > On 2013/08/12 03:06:17, sheu wrote: > > I noted before that I'd like to see these moved to a child-thread section. > Now > > that you have one an put it there, I don't think it should be a problem. > > These three are run on decoder thread. Only ResolutionChangeDestroyBuffers() is > run on child and I moved it to that section... Sorry for the confusion -- I was replying to the old comments on the new CL to keep the comments coming on this patch. All ok now.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/posciak@chromium.org/22590009/27001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/posciak@chromium.org/22590009/27001
Message was sent while issue was closed.
Change committed as 216969 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
