Generate a |release_token| for VideoFrames with destruction observers. [chromium/src : main]

0 views
Skip to first unread message

Andres Calderon Jaramillo (Gerrit)

unread,
Jul 1, 2022, 11:31:59 PMJul 1
to feature-me...@chromium.org, poscia...@chromium.org, xhwang...@chromium.org, Pilar Molina Lopez, Chromium LUCI CQ, Dan Sanders, chromium...@chromium.org

Attention is currently required from: Dan Sanders.

Patch set 3:Commit-Queue +1

View Change

1 comment:

  • File media/mojo/services/mojo_video_decoder_service.cc:

    • Patch Set #1, Line 418: if ((frame->HasReleaseMailboxCB() || frame->HasGpuMemoryBuffer()

      Looks exactly correct to me, except that you also need to check |mailbox_holders_and_gmb_release_cb_ […]

      Ok, I think the latest patchset has your suggestions.

      I can't say I'm fully comfortable with this though :) mostly because of the uncertainty around thread safety: it feels like even if ToT is thread-unsafe, it's only because of a race between `AddDestructionObserver()` and the destructor. I think that may be an easier failure to detect compared to potential defects introduced by this CL: I'm not only adding another possible point of failure, but if ToT is thread-unsafe, I may be adding more undefined behavior because I'm adding a call to std::vector::empty() which may race with std::vector::push_back() ([this answer](https://stackoverflow.com/a/4205568) made me worry about undefined behavior, not just raciness).

      Would there be anything safer? Maybe telling the MojoVideoDecoderService at construction-time to always register frames? Or maybe adding a lock around `done_callbacks_` if only to rule out undefined behavior?

To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I7d4e2dfed07df3b49dd9f29d278314e2450ef08d
Gerrit-Change-Number: 3741131
Gerrit-PatchSet: 3
Gerrit-Owner: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Dan Sanders <sand...@chromium.org>
Gerrit-CC: Pilar Molina Lopez <pmolin...@chromium.org>
Gerrit-Attention: Dan Sanders <sand...@chromium.org>
Gerrit-Comment-Date: Sat, 02 Jul 2022 03:31:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Dan Sanders <sand...@chromium.org>
Comment-In-Reply-To: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-MessageType: comment

Dan Sanders (Gerrit)

unread,
Jul 11, 2022, 5:01:40 PMJul 11
to Andres Calderon Jaramillo, feature-me...@chromium.org, poscia...@chromium.org, xhwang...@chromium.org, Pilar Molina Lopez, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Andres Calderon Jaramillo.

Patch set 4:Code-Review +1

View Change

1 comment:

  • File media/mojo/services/mojo_video_decoder_service.cc:

    • Ok, I think the latest patchset has your suggestions. […]

      I did search through the code for calls to AddDestructionObserver() and didn't find anything suspicious, I think this change will be fine.

      FWIW if it came to it I'd rather have an inverse lock to freeze the state once configuration of the frame is done. I'd make a regular builder but I'm not sure if I can make the Mojo integration ergonomic with that.

To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I7d4e2dfed07df3b49dd9f29d278314e2450ef08d
Gerrit-Change-Number: 3741131
Gerrit-PatchSet: 4
Gerrit-Owner: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Dan Sanders <sand...@chromium.org>
Gerrit-CC: Pilar Molina Lopez <pmolin...@chromium.org>
Gerrit-Attention: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Comment-Date: Mon, 11 Jul 2022 21:01:31 +0000

Andres Calderon Jaramillo (Gerrit)

unread,
Jul 13, 2022, 11:43:30 PMJul 13
to feature-me...@chromium.org, poscia...@chromium.org, xhwang...@chromium.org, Dan Sanders, Pilar Molina Lopez, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Dan Sanders.

Patch set 5:Commit-Queue +1

View Change

1 comment:

  • File media/mojo/services/mojo_video_decoder_service.cc:

    • Sounds good! I went ahead and also added the following:

      ```
      // WARNING: This method is not thread safe; it should only be called if you
      // are still the only owner of this VideoFrame.
      ```

      To `HasReleaseMailboxCB()` and `HasDestructionObservers()` which seems to make sense because if the setters are not thread safe, then the accessors shouldn't be either. What do you think?

      Additionally, can you elaborate more on what you meant by this:

    • FWIW if it came to it I'd rather have an inverse lock to freeze the state once configuration of the frame is done.

    • I'm not sure I understood.

To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I7d4e2dfed07df3b49dd9f29d278314e2450ef08d
Gerrit-Change-Number: 3741131
Gerrit-PatchSet: 5
Gerrit-Owner: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Dan Sanders <sand...@chromium.org>
Gerrit-CC: Pilar Molina Lopez <pmolin...@chromium.org>
Gerrit-Attention: Dan Sanders <sand...@chromium.org>
Gerrit-Comment-Date: Thu, 14 Jul 2022 03:43:18 +0000

Dan Sanders (Gerrit)

unread,
Jul 14, 2022, 1:35:03 PMJul 14
to Andres Calderon Jaramillo, feature-me...@chromium.org, poscia...@chromium.org, xhwang...@chromium.org, Pilar Molina Lopez, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Andres Calderon Jaramillo.

Patch set 5:Code-Review +1

View Change

1 comment:

  • File media/mojo/services/mojo_video_decoder_service.cc:

    • if the setters are not thread safe, then the accessors shouldn't be either. What do you think?

      The setters are not safe *because* the accessors are called on multiple threads. The model is that this data is read-only after initialization.

      FWIW if it came to it I'd rather have an inverse lock [...]


    • I'm not sure I understood.

    • This is a theoretical proposal to be able to seal media::VideoFrames so that the 'read-only after initialization' property is enforced by the setters.

To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I7d4e2dfed07df3b49dd9f29d278314e2450ef08d
Gerrit-Change-Number: 3741131
Gerrit-PatchSet: 5
Gerrit-Owner: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Dan Sanders <sand...@chromium.org>
Gerrit-CC: Pilar Molina Lopez <pmolin...@chromium.org>
Gerrit-Attention: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Comment-Date: Thu, 14 Jul 2022 17:34:55 +0000

Andres Calderon Jaramillo (Gerrit)

unread,
Jul 14, 2022, 6:36:23 PMJul 14
to feature-me...@chromium.org, poscia...@chromium.org, xhwang...@chromium.org, Dan Sanders, Pilar Molina Lopez, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Dan Sanders.

Patch set 6:Commit-Queue +1

View Change

2 comments:

  • File media/base/video_frame.cc:

    • Patch Set #5, Line 1322:

      bool VideoFrame::HasDestructionObservers() const {
      return wrapped_frame_ ? wrapped_frame_->HasDestructionObservers()
      : HasReleaseMailboxCB() || !done_callbacks_.empty();
      }

      I just noticed something:

      `SetReleaseMailboxCB()` and `SetReleaseMailboxAndGpuMemoryBufferCB()` are not allowed to be called for a frame that wraps another frame. Therefore, `HasReleaseMailboxCB()` is well defined: it should not be possible for an outer frame to have such a callback while the inner frame doesn't.

      `AddDestructionObserver()` is different because it's possible for the outer frame to have destruction observers while the inner frame doesn't. I think `HasDestructionObservers()` should return true for such a situation.

      I went ahead and changed this to account for that situation (it's also consistent with the documentation for the method in the .h). I also added a unit test for that.

  • File media/mojo/services/mojo_video_decoder_service.cc:

To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I7d4e2dfed07df3b49dd9f29d278314e2450ef08d
Gerrit-Change-Number: 3741131
Gerrit-PatchSet: 6
Gerrit-Owner: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Dan Sanders <sand...@chromium.org>
Gerrit-CC: Pilar Molina Lopez <pmolin...@chromium.org>
Gerrit-Attention: Dan Sanders <sand...@chromium.org>
Gerrit-Comment-Date: Thu, 14 Jul 2022 22:36:06 +0000

Dan Sanders (Gerrit)

unread,
Jul 14, 2022, 7:12:54 PMJul 14
to Andres Calderon Jaramillo, feature-me...@chromium.org, poscia...@chromium.org, xhwang...@chromium.org, Pilar Molina Lopez, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Andres Calderon Jaramillo.

Patch set 6:Code-Review +1

View Change

2 comments:

  • File media/base/video_frame.cc:

    • Patch Set #5, Line 1322:

      bool VideoFrame::HasDestructionObservers() const {
      return wrapped_frame_ ? wrapped_frame_->HasDestructionObservers()
      : HasReleaseMailboxCB() || !done_callbacks_.empty();
      }

    • I just noticed something: […]

      Good catch, I totally missed that error!

  • File media/mojo/services/mojo_video_decoder_service.cc:

    • > The setters are not safe *because* the accessors are called on multiple threads. […]

      Oops, I suppose these didn't show up with a symbol search so I missed them with my earlier query.

      OOPVideoDecoder: This should be safe since OnVideoFrameDecoded() is called by Mojo while the |frame| has just been deserialized and therefore is still uniquely-owned.

      MailboxVideoFrameConverter: This is more suspicious because it's difficult to prove the frame hasn't also been referenced elsewhere. The standard approach here is to use WrapVideoFrame(), and then the registration would live as long as the wrapper instead of the |origin_frame|.

To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I7d4e2dfed07df3b49dd9f29d278314e2450ef08d
Gerrit-Change-Number: 3741131
Gerrit-PatchSet: 6
Gerrit-Owner: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Dan Sanders <sand...@chromium.org>
Gerrit-CC: Pilar Molina Lopez <pmolin...@chromium.org>
Gerrit-Attention: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Comment-Date: Thu, 14 Jul 2022 23:12:47 +0000

Andres Calderon Jaramillo (Gerrit)

unread,
Jul 14, 2022, 8:28:38 PMJul 14
to feature-me...@chromium.org, poscia...@chromium.org, xhwang...@chromium.org, Dan Sanders, Pilar Molina Lopez, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Dan Sanders.

View Change

1 comment:

  • File media/mojo/services/mojo_video_decoder_service.cc:

    • The standard approach here is to use WrapVideoFrame(), and then the registration would live as long as the wrapper instead of the |origin_frame|.

    • The reason we add this observer to the inner frame (i.e., the GMB frame given out by our pool) is that we want to keep the corresponding SharedImage created by the MailboxVideoFrameConverter alive for as long as the GMB frame lives. So, if the wrapper of that frame dies, we want it to be returned to the pool, but we also want the SharedImage to survive so that we can re-use it if we see that GMB frame again.

To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I7d4e2dfed07df3b49dd9f29d278314e2450ef08d
Gerrit-Change-Number: 3741131
Gerrit-PatchSet: 6
Gerrit-Owner: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Dan Sanders <sand...@chromium.org>
Gerrit-CC: Pilar Molina Lopez <pmolin...@chromium.org>
Gerrit-Attention: Dan Sanders <sand...@chromium.org>
Gerrit-Comment-Date: Fri, 15 Jul 2022 00:28:27 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No

Dan Sanders (Gerrit)

unread,
Jul 25, 2022, 2:04:31 PMJul 25
to Andres Calderon Jaramillo, feature-me...@chromium.org, poscia...@chromium.org, xhwang...@chromium.org, Pilar Molina Lopez, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Andres Calderon Jaramillo.

View Change

1 comment:

  • File media/mojo/services/mojo_video_decoder_service.cc:

    • So, if the wrapper of that frame dies, we want it to be returned to the pool, but we also want the SharedImage to survive so that we can re-use it if we see that GMB frame again.

      This makes some sense, but I would recommend either combining the pools into one that has knowledge of both things, or creating a new pooling layer to hold these modified frames. It also seems possible to create a pool just for the SharedImages and map them to new frames as necessary?

      For things that are more like 'cacheable value computed from a VideoFrame', I would consider adding locked members to VideoFrame, but so far we don't have anything quite like that.

To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I7d4e2dfed07df3b49dd9f29d278314e2450ef08d
Gerrit-Change-Number: 3741131
Gerrit-PatchSet: 6
Gerrit-Owner: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Dan Sanders <sand...@chromium.org>
Gerrit-CC: Pilar Molina Lopez <pmolin...@chromium.org>
Gerrit-Attention: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Comment-Date: Mon, 25 Jul 2022 18:04:22 +0000

Dan Sanders (Gerrit)

unread,
Jul 25, 2022, 2:20:38 PMJul 25
to Andres Calderon Jaramillo, feature-me...@chromium.org, poscia...@chromium.org, xhwang...@chromium.org, Pilar Molina Lopez, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Andres Calderon Jaramillo.

View Change

1 comment:

  • File media/mojo/services/mojo_video_decoder_service.cc:

    • It also seems possible to create a pool just for the SharedImages and map them to new frames as necessary?

      Ignore that, clearly unworkable.

      I'm not sure which of the sub-pool vs. merge with root pool is better in this case, but it's worth noting that the VDAVideoDecoder/PictureBufferManager version of this has a pool that maintains both, and wraps them into VideoFrames only right as they are output.

To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I7d4e2dfed07df3b49dd9f29d278314e2450ef08d
Gerrit-Change-Number: 3741131
Gerrit-PatchSet: 6
Gerrit-Owner: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Dan Sanders <sand...@chromium.org>
Gerrit-CC: Pilar Molina Lopez <pmolin...@chromium.org>
Gerrit-Attention: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Comment-Date: Mon, 25 Jul 2022 18:20:30 +0000

Andres Calderon Jaramillo (Gerrit)

unread,
Jul 25, 2022, 5:28:40 PMJul 25
to feature-me...@chromium.org, poscia...@chromium.org, xhwang...@chromium.org, Dan Sanders, Pilar Molina Lopez, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Dan Sanders.

View Change

1 comment:

  • File media/mojo/services/mojo_video_decoder_service.cc:

    • > It also seems possible to create a pool just for the SharedImages and map them to new frames as ne […]

      We've also considered a pool that manages SharedImages and underlying buffers together, but that has lost some relevance for us due to the out-of-process video decoding project: in that use case, the underlying buffers are allocated in one process and the SharedImages are created in another (the GPU process).

      That also implies that the particular concern we've been discussing (MailboxVideoFrameConverter adding a destruction observer to a frame that's referenced elsewhere) would be moot once I enable out-of-process decoding by default. There might however be other suspicious cases we haven't discussed hidden in the code.

      With all this considered, I see various courses of action:

      1) Just YOLO it and land this CL: however, at the very least, the documentation I added in media/base/video_frame.h would need to be changed because we know for sure there's a case where the thing that adds the destruction observer is not the only owner of the frame, even though it's probably safe.

      2) Guard the new code I'm adding with the out-of-process decoding feature flag: I don't like this too much because it pollutes the code.

      3) Don't rely on the state of the VideoFrame to decide whether to register it or not -- instead, we could tell the MojoVideoDecoderService at creation time whether to register frames or not.

To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I7d4e2dfed07df3b49dd9f29d278314e2450ef08d
Gerrit-Change-Number: 3741131
Gerrit-PatchSet: 6
Gerrit-Owner: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Dan Sanders <sand...@chromium.org>
Gerrit-CC: Pilar Molina Lopez <pmolin...@chromium.org>
Gerrit-Attention: Dan Sanders <sand...@chromium.org>
Gerrit-Comment-Date: Mon, 25 Jul 2022 21:28:33 +0000

Dan Sanders (Gerrit)

unread,
Jul 25, 2022, 5:52:32 PMJul 25
to Andres Calderon Jaramillo, feature-me...@chromium.org, poscia...@chromium.org, xhwang...@chromium.org, Pilar Molina Lopez, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Andres Calderon Jaramillo.

View Change

1 comment:

  • File media/mojo/services/mojo_video_decoder_service.cc:

    • We've also considered a pool that manages SharedImages and underlying buffers together, but that has […]

      (1) is basically how things are working right now so I'm basically okay with that plan. We can add locking if it turns out to be necessary.

      (3) is fine, although I'm not sure I understand how it's sufficient.

To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I7d4e2dfed07df3b49dd9f29d278314e2450ef08d
Gerrit-Change-Number: 3741131
Gerrit-PatchSet: 6
Gerrit-Owner: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Dan Sanders <sand...@chromium.org>
Gerrit-CC: Pilar Molina Lopez <pmolin...@chromium.org>
Gerrit-Attention: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Comment-Date: Mon, 25 Jul 2022 21:52:24 +0000

Andres Calderon Jaramillo (Gerrit)

unread,
Jul 25, 2022, 6:38:39 PMJul 25
to feature-me...@chromium.org, poscia...@chromium.org, xhwang...@chromium.org, Dan Sanders, Pilar Molina Lopez, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Dan Sanders.

View Change

1 comment:

  • File media/mojo/services/mojo_video_decoder_service.cc:

    • (1) is basically how things are working right now so I'm basically okay with that plan. We can add locking if it turns out to be necessary.

    • (3) is fine, although I'm not sure I understand how it's sufficient.

To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I7d4e2dfed07df3b49dd9f29d278314e2450ef08d
Gerrit-Change-Number: 3741131
Gerrit-PatchSet: 6
Gerrit-Owner: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Dan Sanders <sand...@chromium.org>
Gerrit-CC: Pilar Molina Lopez <pmolin...@chromium.org>
Gerrit-Attention: Dan Sanders <sand...@chromium.org>
Gerrit-Comment-Date: Mon, 25 Jul 2022 22:38:31 +0000

Dan Sanders (Gerrit)

unread,
Jul 25, 2022, 6:49:26 PMJul 25
to Andres Calderon Jaramillo, feature-me...@chromium.org, poscia...@chromium.org, xhwang...@chromium.org, Pilar Molina Lopez, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Andres Calderon Jaramillo.

View Change

1 comment:

  • File media/mojo/services/mojo_video_decoder_service.cc:

    • > (1) is basically how things are working right now so I'm basically okay with that plan. […]

      Oh I see. The problem of late-adding a destruction observer still exists, but it doesn't affect this code any more.

      That's fine, the "ugly" part is that you need a way to configure it, which probably means adding another getter method to the media::VideoDecoder interface so that MojoVideoDecoderService can inspect it.

To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I7d4e2dfed07df3b49dd9f29d278314e2450ef08d
Gerrit-Change-Number: 3741131
Gerrit-PatchSet: 6
Gerrit-Owner: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Dan Sanders <sand...@chromium.org>
Gerrit-CC: Pilar Molina Lopez <pmolin...@chromium.org>
Gerrit-Attention: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Comment-Date: Mon, 25 Jul 2022 22:49:16 +0000

Andres Calderon Jaramillo (Gerrit)

unread,
Jul 27, 2022, 5:49:40 PMJul 27
to feature-me...@chromium.org, poscia...@chromium.org, xhwang...@chromium.org, Dan Sanders, Pilar Molina Lopez, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Dan Sanders.

View Change

1 comment:

  • File media/mojo/services/mojo_video_decoder_service.cc:

    • Oh I see. The problem of late-adding a destruction observer still exists, but it doesn't affect this code any more.

    • That's fine, the "ugly" part is that you need a way to configure it, which probably means adding another getter method to the media::VideoDecoder interface so that MojoVideoDecoderService can inspect it.

To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I7d4e2dfed07df3b49dd9f29d278314e2450ef08d
Gerrit-Change-Number: 3741131
Gerrit-PatchSet: 6
Gerrit-Owner: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Dan Sanders <sand...@chromium.org>
Gerrit-CC: Pilar Molina Lopez <pmolin...@chromium.org>
Gerrit-Attention: Dan Sanders <sand...@chromium.org>
Gerrit-Comment-Date: Wed, 27 Jul 2022 21:49:31 +0000

Dan Sanders (Gerrit)

unread,
Jul 27, 2022, 5:58:34 PMJul 27
to Andres Calderon Jaramillo, feature-me...@chromium.org, poscia...@chromium.org, xhwang...@chromium.org, Pilar Molina Lopez, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Andres Calderon Jaramillo.

View Change

1 comment:

  • File media/mojo/services/mojo_video_decoder_service.cc:

    • > Oh I see. […]

      It could be done that way, but it's the MojoMediaClient that then picks a decoder and the decoder that cares about the frame lifetime; I'd rather not spread the information across extra layers.

To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I7d4e2dfed07df3b49dd9f29d278314e2450ef08d
Gerrit-Change-Number: 3741131
Gerrit-PatchSet: 6
Gerrit-Owner: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Dan Sanders <sand...@chromium.org>
Gerrit-CC: Pilar Molina Lopez <pmolin...@chromium.org>
Gerrit-Attention: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Comment-Date: Wed, 27 Jul 2022 21:58:24 +0000
Reply all
Reply to author
Forward
0 new messages