Add Page.startScreenRecording/stopScreenRecording DevTools commands [chromium/src : main]
Alex Rudenko (Gerrit)
Alex Rudenko added 4 comments![]( "Open in Gerrit")
base::span<const uint8_t> mapping_memory(it->second.read_only_mapping);WrapExternalData typically expects a mutable `uint8_t*` or `base::span<uint8_t>` for the data argument. Passing `mapping_memory` (which is a `base::span<const uint8_t>`) here will likely result in a compilation error.
For `read_only_mapping`, you might need to use a `const_cast` or a `WrapReadOnlyExternalData` equivalent if one exists. Similarly, for `writable_mapping` below, you should construct a `base::span<uint8_t>` directly instead of `base::span<const uint8_t>`.
auto audio_bus =There is no need to copy `audio_source` into a temporary `audio_bus` here. `media::AudioBuffer::CopyFrom` creates its own internal copy of the data. You can pass `audio_source` directly to `CopyFrom` to avoid the extra allocation and copy overhead.
gfx::Size fallback_size(max_width_ > 0 ? max_width_ : 800,The comment above states we should only inject a blank frame if *no* frames were received, but this code currently injects a black frame unconditionally on every `Stop()`.
You likely need to add a boolean flag (e.g. `has_received_frames_`), set it to true in `OnFrameFromVideoConsumer`, and check it here before injecting the fallback frame.
If the client calls `StopScreenRecording` multiple times before the first stop completes (i.e. before the async Mojo closure resets `media_recorder_`), `media_recorder_->Stop()` will be called again with a new callback. This overwrites `on_stop_callback_` inside `MediaRecorder`, dropping the first callback and causing it to return an Internal Error due to its destructor firing without a response.
It would be much cleaner to take ownership of the recorder and keep it alive in the closure, instantly clearing `media_recorder_` so subsequent calls return the "No active screen recording" error correctly:
```cpp
auto recorder = std::move(media_recorder_);
recorder->Stop(base::BindOnce(
[](std::unique_ptr<MediaRecorder> recorder,
std::unique_ptr<StopScreenRecordingCallback> callback,
std::string stream) {
if (callback) {
callback->sendSuccess(stream);
}
},
std::move(recorder), std::move(callback)));
```
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 4 comments![]( "Open in Gerrit")
base::span<const uint8_t> mapping_memory(it->second.read_only_mapping);WrapExternalData typically expects a mutable `uint8_t*` or `base::span<uint8_t>` for the data argument. Passing `mapping_memory` (which is a `base::span<const uint8_t>`) here will likely result in a compilation error.
For `read_only_mapping`, you might need to use a `const_cast` or a `WrapReadOnlyExternalData` equivalent if one exists. Similarly, for `writable_mapping` below, you should construct a `base::span<uint8_t>` directly instead of `base::span<const uint8_t>`.
Done
There is no need to copy `audio_source` into a temporary `audio_bus` here. `media::AudioBuffer::CopyFrom` creates its own internal copy of the data. You can pass `audio_source` directly to `CopyFrom` to avoid the extra allocation and copy overhead.
Done
gfx::Size fallback_size(max_width_ > 0 ? max_width_ : 800,The comment above states we should only inject a blank frame if *no* frames were received, but this code currently injects a black frame unconditionally on every `Stop()`.
You likely need to add a boolean flag (e.g. `has_received_frames_`), set it to true in `OnFrameFromVideoConsumer`, and check it here before injecting the fallback frame.
Done
If the client calls `StopScreenRecording` multiple times before the first stop completes (i.e. before the async Mojo closure resets `media_recorder_`), `media_recorder_->Stop()` will be called again with a new callback. This overwrites `on_stop_callback_` inside `MediaRecorder`, dropping the first callback and causing it to return an Internal Error due to its destructor firing without a response.
It would be much cleaner to take ownership of the recorder and keep it alive in the closure, instantly clearing `media_recorder_` so subsequent calls return the "No active screen recording" error correctly:
```cpp
auto recorder = std::move(media_recorder_);
recorder->Stop(base::BindOnce(
[](std::unique_ptr<MediaRecorder> recorder,
std::unique_ptr<StopScreenRecordingCallback> callback,
std::string stream) {
if (callback) {
callback->sendSuccess(stream);
}
},
std::move(recorder), std::move(callback)));
```
Done
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
PTAL. Let me know if you prefer me to split it in some way. The main logic is in media_recorder.cc that manages video and audio capture and sends data to the encoding service. The rest is tests/wiring.
Related details
- Danil Somsikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Somsikov (Gerrit)
Danil Somsikov added 4 comments![]( "Open in Gerrit")
#if !BUILDFLAG(ENABLE_AV1_DECODER)The StartScreenRecording implementation requires `ENABLE_LIBAOM` to record video, but this test checks `ENABLE_AV1_DECODER`. The test might fail on platforms where the AV1 decoder is enabled but the libaom encoder is not. You should check for `ENABLE_LIBAOM` instead.
#if BUILDFLAG(ENABLE_LIBAOM)The stream file and encoding service process are created unconditionally above, before checking `#if BUILDFLAG(ENABLE_LIBAOM)`. If AV1 encoding is not enabled, this returns an error but leaks the created service process and stream file. You should move the `#if BUILDFLAG(ENABLE_LIBAOM)` check to the beginning of the function.
base::span<const uint8_t> mapping_memory(it->second.read_only_mapping);ReadOnlySharedMemoryMapping and WritableSharedMemoryMapping cannot be passed directly to the `base::span` constructor; you must use `.GetMemoryAsSpan<uint8_t>()`.
Furthermore, `media::VideoFrame::WrapExternalData` expects a mutable pointer or `base::span<uint8_t>`. Passing a `span<const uint8_t>` for the read-only mapping will result in a compilation error. You need to cast away the constness of the memory for the read-only mapping to satisfy `WrapExternalData`.
client_buffers_.erase(buffer_id);Erasing the buffer here destroys the underlying `SharedMemoryMapping`. If a `VideoFrame` is still using this memory (e.g., waiting to be encoded asynchronously by the encoding service), it will read from unmapped memory, causing a use-after-free crash.
You probably should wrap `ClientBuffer` in a ref-counted wrapper (e.g., `base::RefCountedThreadSafe`) and capture a reference to it in the `VideoFrame`'s destruction observer (`AddDestructionObserver` in `OnFrameReadyInBuffer`).
Related details
- Alex Rudenko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 4 comments![]( "Open in Gerrit")
#if !BUILDFLAG(ENABLE_AV1_DECODER)The StartScreenRecording implementation requires `ENABLE_LIBAOM` to record video, but this test checks `ENABLE_AV1_DECODER`. The test might fail on platforms where the AV1 decoder is enabled but the libaom encoder is not. You should check for `ENABLE_LIBAOM` instead.
Done
#if BUILDFLAG(ENABLE_LIBAOM)The stream file and encoding service process are created unconditionally above, before checking `#if BUILDFLAG(ENABLE_LIBAOM)`. If AV1 encoding is not enabled, this returns an error but leaks the created service process and stream file. You should move the `#if BUILDFLAG(ENABLE_LIBAOM)` check to the beginning of the function.
Done
base::span<const uint8_t> mapping_memory(it->second.read_only_mapping);ReadOnlySharedMemoryMapping and WritableSharedMemoryMapping cannot be passed directly to the `base::span` constructor; you must use `.GetMemoryAsSpan<uint8_t>()`.
Furthermore, `media::VideoFrame::WrapExternalData` expects a mutable pointer or `base::span<uint8_t>`. Passing a `span<const uint8_t>` for the read-only mapping will result in a compilation error. You need to cast away the constness of the memory for the read-only mapping to satisfy `WrapExternalData`.
Applied the `.GetMemoryAsSpan<uint8_t>()` suggestion but the comment about media::VideoFrame::WrapExternalData does not appear to be accurate: it accepts `base::span<const uint8_t>` (https://source.chromium.org/chromium/chromium/src/+/main:media/base/video_frame.cc;l=580;drc=e87601ce38a77979426caf8d9d75f8019bbbef1d). So I think we do not need to cast away constness.
client_buffers_.erase(buffer_id);Erasing the buffer here destroys the underlying `SharedMemoryMapping`. If a `VideoFrame` is still using this memory (e.g., waiting to be encoded asynchronously by the encoding service), it will read from unmapped memory, causing a use-after-free crash.
You probably should wrap `ClientBuffer` in a ref-counted wrapper (e.g., `base::RefCountedThreadSafe`) and capture a reference to it in the `VideoFrame`'s destruction observer (`AddDestructionObserver` in `OnFrameReadyInBuffer`).
- Danil Somsikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Somsikov (Gerrit)
Danil Somsikov added 2 comments![]( "Open in Gerrit")
media is then sent to a new DevToolsMediaEncodingServer service forWhere is this defined?
remote->StartRecording(client_receiver_.BindNewPipeAndPassRemote(), max_width,Following up on the design doc discussion regarding fragmented MP4 (fMP4): were we able to enable fMP4 format for the stream output here?
Related details
- Alex Rudenko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
media is then sent to a new DevToolsMediaEncodingServer service forAlex RudenkoWhere is this defined?
This was added in https://crrev.com/c/7951617.
Related details
- Danil Somsikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
remote->StartRecording(client_receiver_.BindNewPipeAndPassRemote(), max_width,Following up on the design doc discussion regarding fragmented MP4 (fMP4): were we able to enable fMP4 format for the stream output here?
I added the fMP4 exploration to a follow up in the design doc since the current use cases are motivated by saving the video stream to a file in ChromeDriver/Puppeteer. So I have not tried fMP4 yet. The streaming via a video tag already works with the current mp4 encoder (test script: https://paste.googleplex.com/5961887689867264) if the page produces keyframes (we might want to tweak how often the encoder flushes once we integrate streaming in our products).
Related details
- Danil Somsikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Somsikov (Gerrit)
Danil Somsikov added 1 comment![]( "Open in Gerrit")
Ok, this works, but this is obviously too big. Can we have (a subset) if a media recorder in a standalone CL with unit tests?
Related details
- Alex Rudenko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
remote->StartRecording(client_receiver_.BindNewPipeAndPassRemote(), max_width,Alex RudenkoFollowing up on the design doc discussion regarding fragmented MP4 (fMP4): were we able to enable fMP4 format for the stream output here?
I added the fMP4 exploration to a follow up in the design doc since the current use cases are motivated by saving the video stream to a file in ChromeDriver/Puppeteer. So I have not tried fMP4 yet. The streaming via a video tag already works with the current mp4 encoder (test script: https://paste.googleplex.com/5961887689867264) if the page produces keyframes (we might want to tweak how often the encoder flushes once we integrate streaming in our products).
- Danil Somsikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |