[Cast] Add BufferIdManager [chromium/src : master]
Ryan Keane (Gerrit)
Attention is currently required from: Alex Leung.
Ryan Keane would like Alex Leung to review this change.
[Cast] Add BufferIdManager
This CL introduces the concept of a "BufferId", or a unique ID assigned
to each PushBuffer call. Additionally, it introduces the BufferIdManager
class, a utility class used to approximate these IDs at a given time.
This functionality is required for audio-audio sync as defined in:
go/cast-media-chromium-runtime
Bug: b/171495618
Change-Id: I3bec4b6d442bc86f22d78bf7ce155e2f65f7090c
---
M chromecast/media/cma/BUILD.gn
M chromecast/media/cma/backend/proxy/BUILD.gn
A chromecast/media/cma/backend/proxy/buffer_id_manager.cc
A chromecast/media/cma/backend/proxy/buffer_id_manager.h
A chromecast/media/cma/backend/proxy/buffer_id_manager_unittest.cc
5 files changed, 285 insertions(+), 0 deletions(-)
To view, visit change 2580782. To unsubscribe, or for help writing mail filters, visit settings.
Ryan Keane (Gerrit)
Attention is currently required from: Alex Leung.
1 comment:
Patchset:
@alexleung Please confirm that the logic is in-sync with what you expect on the other side of the RPC
To view, visit change 2580782. To unsubscribe, or for help writing mail filters, visit settings.
Alex Leung (Gerrit)
Attention is currently required from: Ryan Keane.
3 comments:
File chromecast/media/cma/backend/proxy/buffer_id_manager.h:
Patch Set #3, Line 26:
uint64_twe use int64_t.
File chromecast/media/cma/backend/proxy/buffer_id_manager.cc:
Patch Set #3, Line 20:
g_next_idThis can be reset each time Stop() is called.
Patch Set #3, Line 65:
g_next_idsince this is int64_t, you will need to handle the wrap to 0.
To view, visit change 2580782. To unsubscribe, or for help writing mail filters, visit settings.
Ryan Keane (Gerrit)
Attention is currently required from: Alex Leung.
Patch set 4:Commit-Queue +1
3 comments:
File chromecast/media/cma/backend/proxy/buffer_id_manager.h:
Patch Set #3, Line 26:
uint64_twe use int64_t.
Done
File chromecast/media/cma/backend/proxy/buffer_id_manager.cc:
Patch Set #3, Line 20:
g_next_idThis can be reset each time Stop() is called.
Is this a requirement for the other side, or just a suggestion here? IMHO having a monotonically increasing ID could make some debugging easier. Alternatively, to that point, WDYT about making this an instance variable but initializing it to a random number?
My goal here is to avoid having the same PushBuffer ID show up twice, so that debug logs never have that added complexity
Patch Set #3, Line 65:
g_next_idsince this is int64_t, you will need to handle the wrap to 0.
Done
To view, visit change 2580782. To unsubscribe, or for help writing mail filters, visit settings.
Alex Leung (Gerrit)
Attention is currently required from: Yuchen Liu, Ryan Keane.
Alex Leung would like Yuchen Liu to review this change authored by Ryan Keane.
[Cast] Add BufferIdManager
This CL introduces the concept of a "BufferId", or a unique ID assigned
to each PushBuffer call. Additionally, it introduces the BufferIdManager
class, a utility class used to approximate these IDs at a given time.
This functionality is required for audio-audio sync as defined in:
go/cast-media-chromium-runtime
Bug: b/171495618
Change-Id: I3bec4b6d442bc86f22d78bf7ce155e2f65f7090c
---
M chromecast/media/cma/BUILD.gn
M chromecast/media/cma/backend/proxy/BUILD.gn
A chromecast/media/cma/backend/proxy/buffer_id_manager.cc
A chromecast/media/cma/backend/proxy/buffer_id_manager.h
A chromecast/media/cma/backend/proxy/buffer_id_manager_unittest.cc
5 files changed, 310 insertions(+), 0 deletions(-)
Alex Leung (Gerrit)
Attention is currently required from: Yuchen Liu, Ryan Keane.
1 comment:
Patchset:
LGTM
To view, visit change 2580782. To unsubscribe, or for help writing mail filters, visit settings.
Yuchen Liu (Gerrit)
Attention is currently required from: Ryan Keane.
5 comments:
Patchset:
The patch lg, but it's worth to see how will it be used in other changes.
File chromecast/media/cma/backend/proxy/buffer_id_manager.h:
// The ID of the current buffer.
BufferId id;
// The time in seconds of playback (as number of buffers of playback divided
// by number of buffer per second) for this buffer. This must be kept track
// of rather than the number of bytes in the source buffer to allow for
// changes to the audio config part way through playback.
double playback_time_in_seconds;Give them some initial value.
File chromecast/media/cma/backend/proxy/buffer_id_manager.cc:
Patch Set #4, Line 41:
BufferIdQueue g_buffer_id_qeueue;nit: you can make this a singleton with base::NoDestructor.
// Helper to convert from Chromium callback type (OnceCallback) to Chromecast's
// TaskRunner's Task type.
class OnceCallbackTask : public TaskRunner::Task {
public:
OnceCallbackTask(base::OnceClosure callback)
: callback_(std::move(callback)) {}
~OnceCallbackTask() override = default;
private:
// TaskRunner::Task overrides:
void Run() override { std::move(callback_).Run(); }
base::OnceClosure callback_;
};I think I see this in another change as well. Consider move them to a common file.
Patch Set #4, Line 83:
doublestatic_cast.
To view, visit change 2580782. To unsubscribe, or for help writing mail filters, visit settings.
Ryan Keane (Gerrit)
Attention is currently required from: Yuchen Liu.
Patch set 5:Commit-Queue +1
5 comments:
Patchset:
The patch lg, but it's worth to see how will it be used in other changes.
+1. Sorry about that. Hadn't intended to send it out for full review until I had the follow-up patch up too.
File chromecast/media/cma/backend/proxy/buffer_id_manager.h:
// The ID of the current buffer.
BufferId id;
// The time in seconds of playback (as number of buffers of playback divided
// by number of buffer per second) for this buffer. This must be kept track
// of rather than the number of bytes in the source buffer to allow for
// changes to the audio config part way through playback.
double playback_time_in_seconds;Give them some initial value.
Done. Giving them an invalid default value that I DCHECK against later
Do you think it's worth defining a kInvalidBufferId kInvalidPlaybackTime as BufferInfo constants in the header file? I couldn't decide and went with no for now because I haven't run across that pattern much in chromecast/ code.
File chromecast/media/cma/backend/proxy/buffer_id_manager.cc:
Patch Set #4, Line 41:
BufferIdQueue g_buffer_id_qeueue;nit: you can make this a singleton with base::NoDestructor.
Done
// Helper to convert from Chromium callback type (OnceCallback) to Chromecast's
// TaskRunner's Task type.
class OnceCallbackTask : public TaskRunner::Task {
public:
OnceCallbackTask(base::OnceClosure callback)
: callback_(std::move(callback)) {}
~OnceCallbackTask() override = default;
private:
// TaskRunner::Task overrides:
void Run() override { std::move(callback_).Run(); }
base::OnceClosure callback_;
};I think I see this in another change as well. Consider move them to a common file.
Yeah, this is duplicated. Was planning to ask where you suggested for the common code
IMHO it would make sense in chromecast/base/task_runner_utils.h or similar (right next to task_runner_impl.h), but not fully sure about the file structure chromecast/ uses
Patch Set #4, Line 83:
doublestatic_cast.
Done
To view, visit change 2580782. To unsubscribe, or for help writing mail filters, visit settings.
Ryan Keane (Gerrit)
Attention is currently required from: Alex Leung, Yuchen Liu.
Patch set 6:Commit-Queue +1
1 comment:
Patchset:
Added a child CL that shows how this class will be used
To view, visit change 2580782. To unsubscribe, or for help writing mail filters, visit settings.
Ryan Keane (Gerrit)
Attention is currently required from: Alex Leung, Yuchen Liu.
1 comment:
Patchset:
Ping. PTAL. The follow-up CL can come later, but this one needs to get in before I can address b/176252973 or b/175701705
To view, visit change 2580782. To unsubscribe, or for help writing mail filters, visit settings.
Yuchen Liu (Gerrit)
Attention is currently required from: Alex Leung, Ryan Keane.
4 comments:
File chromecast/media/cma/backend/proxy/buffer_id_manager.h:
Patch Set #6, Line 53:
playback_time_in_secondsThe rest of the codes are using microseconds, recommend keep consistency with others. Also, why not use number of buffers instead of time?
File chromecast/media/cma/backend/proxy/buffer_id_manager.cc:
Patch Set #6, Line 29:
return static_cast<int64_t>(next_id_ & kBufferIdMask);Is this to handle overflow? If using uint64_t everywhere, it should be handled automatically.
Patch Set #6, Line 34:
next_id_++;Do you need a lock here? If this is supposed to be called from a single thread, add a thread checker to enforce that.
emplace?
To view, visit change 2580782. To unsubscribe, or for help writing mail filters, visit settings.
Alex Leung (Gerrit)
Attention is currently required from: Yuchen Liu, Ryan Keane.
4 comments:
File chromecast/media/cma/backend/proxy/buffer_id_manager.h:
Patch Set #6, Line 53:
playback_time_in_seconds
The rest of the codes are using microseconds, recommend keep consistency with others. […]
+1. I would use microseconds or base::TimeDelta.
File chromecast/media/cma/backend/proxy/buffer_id_manager.cc:
Patch Set #6, Line 39:
uint64_tShould this be int64_t?
Patch Set #6, Line 42:
base::NoDestructor<BufferIdQueue> g_buffer_id_qeueue;I think we should have a new queue per stream / session.
Where does this get deleted?
To view, visit change 2580782. To unsubscribe, or for help writing mail filters, visit settings.
Ryan Keane (Gerrit)
Attention is currently required from: Alex Leung, Yuchen Liu.
Patch set 7:Commit-Queue +1
8 comments:
File chromecast/media/cma/backend/proxy/buffer_id_manager.h:
Patch Set #6, Line 53:
playback_time_in_secondsThe rest of the codes are using microseconds, recommend keep consistency with others
Done. Updating to microseconds (over TimeDelta for consistency)
Initially used seconds to avoid some of the weirdness that goes along with floating point math (the config gives us samples_per_second not millisecond), but in practice this shouldn't be a problem given we only need this class to be approximate, so readability can take priority
why not use number of buffers instead of time
It's tracked in time rather than buffer count to account for a SetConfig call to update the buffers per second. Per CmaBackendProxy, this must be supported.
File chromecast/media/cma/backend/proxy/buffer_id_manager.cc:
// Helper to convert from Chromium callback type (OnceCallback) to Chromecast's
// TaskRunner's Task type.
class OnceCallbackTask : public TaskRunner::Task {
public:
OnceCallbackTask(base::OnceClosure callback)
: callback_(std::move(callback)) {}
~OnceCallbackTask() override = default;
private:
// TaskRunner::Task overrides:
void Run() override { std::move(callback_).Run(); }
base::OnceClosure callback_;
};
Yeah, this is duplicated. Was planning to ask where you suggested for the common code […]
Will do this in a follow-up CL immediately after this one, so that discussions of where the code should go don't delay checking this one in - this CL is blocking a few others for me
File chromecast/media/cma/backend/proxy/buffer_id_manager.cc:
Patch Set #6, Line 29:
return static_cast<int64_t>(next_id_ & kBufferIdMask);
Is this to handle overflow? If using uint64_t everywhere, it should be handled automatically.
The gRPC API requires a int64_t rather than a uint64_t, so this is the only place it's used. This was the cleanest way I could come up with to handle the overflow case.
Patch Set #6, Line 34:
next_id_++;
Do you need a lock here? If this is supposed to be called from a single thread, add a thread checker […]
Good catch! Added checker
After thinking a bit, I think a sequence checker would be safer - Only one CmaBackendProxy will exist at a time, but there's no reason they would need to be created on the same thread for different instances
Patch Set #6, Line 39:
uint64_tShould this be int64_t?
It is intentionally uint64_t here to allow for overflow without crashing
Adding comment
Patch Set #6, Line 42:
base::NoDestructor<BufferIdQueue> g_buffer_id_qeueue;I think we should have a new queue per stream / session.
The reason I use a singleton here is because all this class is responsible for is pulling out the next-available BufferId (It's not the queue that stores the playback time data). So with a singleton, one session will pick up where the last left off rather than restarting at zero.
Because the other end of the gRPC does not care about the starting buffer id, this shouldn't have any downsides. But the benefit is it allows for simpler debugability. E.g if we cast twice and are logging the buffer ID (I am sure we will be during debugging) we won't need to guess as which session is associated with the PushBuffer call
Alternatively:
- We can have a per-session instance, but initialize it to start at a random uint64_t
- We could add a "MultizoneSessionID" GUID (or similar) to each call. We wouldn't use it cast-core side except for logging purposes, but it would give a different way to differentiate different sessions
WDYT?
emplace?
Done. Had to add a ctor to the struct though which I find a bit sketchy - but agree it's probably worth it for the free performance improvement of emplace over push.
Where does this get deleted?
The Task Runner actually takes ownership of the posted task. Took me a while to figure out while my tests were segfaulting
https://source.chromium.org/chromium/chromium/src/+/master:chromecast/base/task_runner_impl.cc;l=29
To view, visit change 2580782. To unsubscribe, or for help writing mail filters, visit settings.
Alex Leung (Gerrit)
Attention is currently required from: Yuchen Liu, Ryan Keane.
1 comment:
File chromecast/media/cma/backend/proxy/buffer_id_manager.cc:
Patch Set #6, Line 42:
base::NoDestructor<BufferIdQueue> g_buffer_id_qeueue;The reason I use a singleton here is because all this class is responsible for is pulling out the ne […]
The nice thing about having a per-session instance is that there would be a detectable gap in the buffer id's between the tracks. My sample implementation has it restarting at 0 each time and that's quite easy for debugging. Starting at zero is not necessary but does help avoid the wrapping scenario since a max int64_t of buffers is a "very long" time. That said, the server should correctly handle the wrapping scenario.
If you do not want to start at zero, I think a random uint64_t should work. Although, enforcing that the first buffer sent after a Stop() command is 0 is also a nice way to sanity check that the state of server and client match.
To view, visit change 2580782. To unsubscribe, or for help writing mail filters, visit settings.
Ryan Keane (Gerrit)
Attention is currently required from: Alex Leung, Yuchen Liu.
Patch set 8:Commit-Queue +1
1 comment:
File chromecast/media/cma/backend/proxy/buffer_id_manager.cc:
Patch Set #6, Line 42:
base::NoDestructor<BufferIdQueue> g_buffer_id_qeueue;
The nice thing about having a per-session instance is that there would be a detectable gap in the buffer id's between the tracks.
I'm worried that there are bugs where this would be difficult to use - e.g. if we get 100 id=0 buffers in a row, that could be a bug in a lot of places and logging the buffer ID wouldn't actually help us find it at all (which IMHO is the main point of logs). Though I think your suggestion for a random buffer id should fix that.
Although, enforcing that the first buffer sent after a Stop() command is 0 is also a nice way to sanity check that the state of server and client match.
For this reason, I'm actually *against* starting at zero - this will be a public API, so we need to know that the server side will handle calls regardless of how the client chooses to start the PushBuffer ID.
random uint64_t should work
Done
To view, visit change 2580782. To unsubscribe, or for help writing mail filters, visit settings.
Ryan Keane (Gerrit)
Attention is currently required from: Alex Leung, Yuchen Liu.
Patch set 9:Commit-Queue +1
1 comment:
File chromecast/media/cma/backend/proxy/buffer_id_manager.cc:
Patch Set #6, Line 34:
next_id_++;Good catch! Added checker […]
After Alex's follow-up comment, I've moved it to the parent class as this class is now per-instance.
To view, visit change 2580782. To unsubscribe, or for help writing mail filters, visit settings.
Ryan Keane (Gerrit)
Attention is currently required from: Alex Leung, Yuchen Liu.
Patch set 10:Commit-Queue +1
1 comment:
Patchset:
Ping
To view, visit change 2580782. To unsubscribe, or for help writing mail filters, visit settings.
Alex Leung (Gerrit)
Attention is currently required from: Yuchen Liu, Ryan Keane.
1 comment:
Patchset:
LGTM
To view, visit change 2580782. To unsubscribe, or for help writing mail filters, visit settings.
Yuchen Liu (Gerrit)
Commit Message:
Patch Set #10, Line 16:
Bug: b/171495618internal b/171495618
File chromecast/media/cma/backend/proxy/buffer_id_manager.cc:
Patch Set #10, Line 46:
static_cast<uint64_t>(rand());base::RandUint64();
Patch Set #10, Line 55:
next_id_++;Do you need thread checker here?
To view, visit change 2580782. To unsubscribe, or for help writing mail filters, visit settings.
Ryan Keane (Gerrit)
Attention is currently required from: Ryan Keane.
Ryan Keane uploaded patch set #12 to this change.
[Cast] Add BufferIdManager
This CL introduces the concept of a "BufferId", or a unique ID assigned
to each PushBuffer call. Additionally, it introduces the BufferIdManager
class, a utility class used to approximate these IDs at a given time.
This functionality is required for audio-audio sync as defined in:
go/cast-media-chromium-runtime
Bug: internal b/171495618
Change-Id: I3bec4b6d442bc86f22d78bf7ce155e2f65f7090c
---
M chromecast/media/cma/BUILD.gn
M chromecast/media/cma/backend/proxy/BUILD.gn
A chromecast/media/cma/backend/proxy/buffer_id_manager.cc
A chromecast/media/cma/backend/proxy/buffer_id_manager.h
A chromecast/media/cma/backend/proxy/buffer_id_manager_unittest.cc
5 files changed, 347 insertions(+), 0 deletions(-)
To view, visit change 2580782. To unsubscribe, or for help writing mail filters, visit settings.
Ryan Keane (Gerrit)
Attention is currently required from: Yuchen Liu.
Patch set 12:Commit-Queue +1
4 comments:
Commit Message:
Patch Set #10, Line 16:
Bug: b/171495618internal b/171495618
Done
Patchset:
Still investigating the weird UT failure - it passes when run locally
File chromecast/media/cma/backend/proxy/buffer_id_manager.cc:
Patch Set #10, Line 46:
static_cast<uint64_t>(rand());base::RandUint64();
Done
Patch Set #10, Line 55:
next_id_++;Do you need thread checker here?
It shouldn't be needed here. The BufferIdQueue is now just an instance variable inside BufferIdManager, and all public methods of the BufferIdQueue have a sequence checker
Though I can change that to a thread checker if you prefer - just didn't see any reason for the extra restriction
To view, visit change 2580782. To unsubscribe, or for help writing mail filters, visit settings.
Yuchen Liu (Gerrit)
Attention is currently required from: Ryan Keane.
Patch set 12:Code-Review +1
1 comment:
File chromecast/media/cma/backend/proxy/buffer_id_manager.cc:
Patch Set #10, Line 55:
next_id_++;It shouldn't be needed here. […]
Ack
To view, visit change 2580782. To unsubscribe, or for help writing mail filters, visit settings.
mark a. foltz (Gerrit)
Attention is currently required from: Ryan Keane.
12 comments:
Patchset:
I had a few coding style comments - I didn't have time to dig into the algorithm in the design doc, sorry if that answers some of the questions below. I found it a little hard to follow the intention from the API documentation alone.
File chromecast/media/cma/backend/proxy/buffer_id_manager.h:
Patch Set #12, Line 24:
// Tracks the Ids of buffers sent over the gRPC.Can you be a little more specific - which gRPC API is used, and which direction are the buffers sent
Patch Set #12, Line 33:
// Sets the audio config to use for future buffer pushes.I'm not quite sure how this fits in with the Assign/Get APIs below. Does calling this increment the buffer ID?
Patch Set #12, Line 37:
// be called prior to the first call to SetAudioConfig().This method is only called with a frame_size; how does this object know which buffer it applies to?
Patch Set #12, Line 42:
// difference due to a slight mismatch.By "user" do you mean the end user, or the caller of this API?
Patch Set #12, Line 58:
uint64_t next_id_;This is unsigned, while BufferIdManager::BufferId is signed. Are negative BufferIds ok?
Patch Set #12, Line 71:
// allow for changes to the audio config part way through playback.Is this actually a duration? It sounds like it's measuring the intended duration to play out a certain number of buffers.
Patch Set #12, Line 82:
void RepeatedlyPruneBufferInfos();Using a task is okay, but you could also prune when a new buffer is pushed onto the end of the queue. Since you are already pruning when buffer IDs are assigned, do you need the extra repeated task?
Patch Set #12, Line 87:
// Number of bytes per second of media playback. This will change each timebytes per *microsecond*
This is going to be well below 1.0 if the buffers have compressed media. 128 kpbs / 1000000 us = 0.016 bytes/us
Not a big deal, just wanted to point it out.
File chromecast/media/cma/backend/proxy/buffer_id_manager.cc:
Patch Set #12, Line 23:
constexpr int kMicrosecondsPerSecond = 1000000;FWIW, this is also defined in base/time.h
Patch Set #12, Line 27:
class OnceCallbackTask : public TaskRunner::Task {This looks copied from proxy_call_translator.cc. Please pull it out into a separate file so it's not duplicated.
Patch Set #12, Line 48:
return static_cast<int64_t>(next_id_ & kBufferIdMask);So next_id_ will wrap around from 0x7FFFFFFF to 0x00000000... Is that OK?
To view, visit change 2580782. To unsubscribe, or for help writing mail filters, visit settings.
Ryan Keane (Gerrit)
Attention is currently required from: mark a. foltz.
Patch set 13:Commit-Queue +1
11 comments:
File chromecast/media/cma/backend/proxy/buffer_id_manager.h:
Patch Set #12, Line 24:
// Tracks the Ids of buffers sent over the gRPC.Can you be a little more specific - which gRPC API is used, and which direction are the buffers sent
Done. Extensively updated the documentation
PTAL and let me know if this is clearer. Closing comment so please open a new one if not
Patch Set #12, Line 33:
// Sets the audio config to use for future buffer pushes.
I'm not quite sure how this fits in with the Assign/Get APIs below. […]
Documentation updated
This is to calculate the number of bytes per (micro)second that are played out.
Patch Set #12, Line 37:
// be called prior to the first call to SetAudioConfig().This method is only called with a frame_size; how does this object know which buffer it applies to?
Documentation updated
The trick here is that we don't actually need to know the frame it's associated with. We just need to know the ID of the currently playing frame (approximately). The other side of the gRPC is responsible for storing this association. So to calculate that, we just look at amount of time of pending data in the underlying decoder, and do some math with the number of bytes per second to calculate it
Patch Set #12, Line 42:
// difference due to a slight mismatch.By "user" do you mean the end user, or the caller of this API?
Documentation updated
This is referring to the end-user of the product using this runtime
Patch Set #12, Line 58:
uint64_t next_id_;
This is unsigned, while BufferIdManager::BufferId is signed. […]
Negative BufferIds are never expected / are invalid, but int64_t was chosen for the type of the ID on the gRPC. Not ideal, but its workable.
Patch Set #12, Line 71:
// allow for changes to the audio config part way through playback.
Is this actually a duration? It sounds like it's measuring the intended duration to play out a cert […]
Documentation updated - this is a duration.
A duration type should not be used here for consistency with other Cma code
Patch Set #12, Line 82:
void RepeatedlyPruneBufferInfos();
Using a task is okay, but you could also prune when a new buffer is pushed onto the end of the queue […]
Updated as suggested
Good point. I was originally only pruning when we call GetCurrentlyProcessingBuffer(), which is relatively infrequent. But doing so on AssignBuffer too is cleaner and lets us remove the task.
Patch Set #12, Line 87:
// Number of bytes per second of media playback. This will change each time
bytes per *microsecond* […]
+1. I originally coded it as per-second, but reviewers in previous iterations asked me to change to microseconds instead.
File chromecast/media/cma/backend/proxy/buffer_id_manager.cc:
Patch Set #12, Line 23:
constexpr int kMicrosecondsPerSecond = 1000000;
FWIW, this is also defined in base/time. […]
Done. Good to know!
Patch Set #12, Line 27:
class OnceCallbackTask : public TaskRunner::Task {
This looks copied from proxy_call_translator.cc. […]
I mentioned in a previous comment, but I will do this as a follow-up CL to avoid blocking this CL on where-should-this-file-live discussions
Though per your previous comment, I was able to remove the use of tasks entirely, so no longer needed
Patch Set #12, Line 48:
return static_cast<int64_t>(next_id_ & kBufferIdMask);
So next_id_ will wrap around from 0x7FFFFFFF to 0x00000000... […]
Yes, this is intentional. Validated with @alexleung that the other side of the gRPC handles this correctly
To view, visit change 2580782. To unsubscribe, or for help writing mail filters, visit settings.
mark a. foltz (Gerrit)
Attention is currently required from: Ryan Keane.
Patch set 14:Code-Review +1
10 comments:
Patchset:
Code looks good, a few requests to clarify documentation.
File chromecast/media/cma/backend/proxy/buffer_id_manager.h:
Patch Set #14, Line 23:
// an increasing BufferId (with the exception of wrap-around of wrap-around ofwrap-around is repeated
Patch Set #14, Line 25:
// runtime and any playback on the other side of the gRPC Channel.I don't think Cast Core itself is responsible for playback "on the other side" of the gRPC channel. Maybe adjust the wording here?
Patch Set #14, Line 33:
// of playback data currently pending in the AudioDecoder's internal queue.I wonder if the BufferIds could just be assigned by counting the total number of bytes that have been placed into PushBuffer calls - essentially a high water mark for the amount of data sent over gRPC.
However, it sounds like it's important for the buffer IDs to be computed based on the pending duration of audio pushed into the AudioDecoder.
FWIW, the design doc mostly talks about using PTS for A/A sync, and doesn't mention buffer IDs. Maybe I missed that section.
Patch Set #14, Line 44:
BufferIdManager(CmaBackend::AudioDecoder* audio_decoder);Please document lifetime assumption of |audio_decoder|.
Patch Set #14, Line 55:
// not be called prior to the first call to SetAudioConfig().Perhaps this class should take an initial audio config in its ctor. I don't feel strongly though - depends on the calling pattern.
Patch Set #14, Line 58:
// Returns the APPROXIMATE BufferId which is currently being processed. NoteCan you specify what is doing the processing?
Patch Set #14, Line 86:
// The duration of time in microseconds of playback (as number of buffers ofIt sounds like this is computed across multiple buffers, but is only valid for this buffer. Can you clarify?
Patch Set #14, Line 87:
// playback divided by number of buffer per second) for this buffer. Thisbuffers per microsecond?
File chromecast/media/cma/backend/proxy/buffer_id_manager.h:
Patch Set #12, Line 58:
uint64_t next_id_;Negative BufferIds are never expected / are invalid, but int64_t was chosen for the type of the ID o […]
Can you mention the gRPC dependency in the documentation for BufferId? Otherwise it's not clear why there's a difference.
To view, visit change 2580782. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
CQ is trying the patch.
Note: The patchset #16 "Merge in Master" sent to CQ was uploaded after this CL was CR+1-ed.
Reviewer, please verify there is nothing unexpected https://chromium-review.googlesource.com/c/2580782/16
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/2580782/16
Bot data: {"action": "start", "triggered_at": "2021-01-29T08:13:54.0Z", "revision": "8a4f56c8e036f5bcc9bacc923328ebc95a45f18c"}
Ryan Keane (Gerrit)
Patch set 16:Commit-Queue +2
8 comments:
File chromecast/media/cma/backend/proxy/buffer_id_manager.h:
Patch Set #14, Line 23:
// an increasing BufferId (with the exception of wrap-around of wrap-around ofwrap-around is repeated
Done
Patch Set #14, Line 25:
// runtime and any playback on the other side of the gRPC Channel.
I don't think Cast Core itself is responsible for playback "on the other side" of the gRPC channel. […]
Done
Patch Set #14, Line 33:
// of playback data currently pending in the AudioDecoder's internal queue.
I wonder if the BufferIds could just be assigned by counting the total number of bytes that have bee […]
The doc is pretty out-of-date at this point. Though also the design seems to still be in-flux (e.g. we had some pretty big changes to the gRPC on Friday), so I think it's worth waiting to update it until later
FWIW I chatted with Alex a while back and we decided this was the best option
Patch Set #14, Line 44:
BufferIdManager(CmaBackend::AudioDecoder* audio_decoder);Please document lifetime assumption of |audio_decoder|.
Done
Patch Set #14, Line 55:
// not be called prior to the first call to SetAudioConfig().
Perhaps this class should take an initial audio config in its ctor. […]
The AudioConfig isn't set yet at the time of this object's creation - the CmaBackend::AudioDecoder that owns this object has a similar SetConfig() method called at runtime.
We could do lazy initialization, but IMHO this is adding unneeded complexity - especially when we already need a SetConfig method to support updates to the config later in the object's lifetime.
Patch Set #14, Line 58:
// Returns the APPROXIMATE BufferId which is currently being processed. NoteCan you specify what is doing the processing?
Done
Patch Set #14, Line 86:
// The duration of time in microseconds of playback (as number of buffers of
It sounds like this is computed across multiple buffers, but is only valid for this buffer. […]
Done
Patch Set #14, Line 87:
// playback divided by number of buffer per second) for this buffer. Thisbuffers per microsecond?
Done
To view, visit change 2580782. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
[Cast] Add BufferIdManager
This CL introduces the concept of a "BufferId", or a unique ID assigned
to each PushBuffer call. Additionally, it introduces the BufferIdManager
class, a utility class used to approximate these IDs at a given time.
This functionality is required for audio-audio sync as defined in:
go/cast-media-chromium-runtime
Bug: internal b/171495618
Change-Id: I3bec4b6d442bc86f22d78bf7ce155e2f65f7090c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2580782
Commit-Queue: Ryan Keane <rwk...@google.com>
Reviewed-by: mark a. foltz <mfo...@chromium.org>
Reviewed-by: Yuchen Liu <yuc...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#848436}
---
M chromecast/media/cma/BUILD.gn
M chromecast/media/cma/backend/proxy/BUILD.gn
A chromecast/media/cma/backend/proxy/buffer_id_manager.cc
A chromecast/media/cma/backend/proxy/buffer_id_manager.h
A chromecast/media/cma/backend/proxy/buffer_id_manager_unittest.cc
5 files changed, 326 insertions(+), 0 deletions(-)
Findit (Gerrit)
Findit has created a revert of this change.
To view, visit change 2580782. To unsubscribe, or for help writing mail filters, visit settings.
Ryan Keane (Gerrit)
Attention is currently required from: Yuchen Liu.
Ryan Keane would like Yuchen Liu to review this change.
[Cast] Add BufferIdManager
This CL introduces the concept of a "BufferId", or a unique ID assigned
to each PushBuffer call. Additionally, it introduces the BufferIdManager
class, a utility class used to approximate these IDs at a given time.
This functionality is required for audio-audio sync as defined in:
go/cast-media-chromium-runtime
This CL un-reverts CL:
https://chromium-review.googlesource.com/c/chromium/src/+/2580782
The issue was that a buildbot runs the unit tests on a non-debug build,
so a DCHECK doesn't fail the bot run.
Bug: internal b/171495618
Change-Id: Iaeb529f0a11ce57264e47e6e9ae4392bf9ad15b0
---
M chromecast/media/cma/BUILD.gn
M chromecast/media/cma/backend/proxy/BUILD.gn
A chromecast/media/cma/backend/proxy/buffer_id_manager.cc
A chromecast/media/cma/backend/proxy/buffer_id_manager.h
A chromecast/media/cma/backend/proxy/buffer_id_manager_unittest.cc
5 files changed, 322 insertions(+), 0 deletions(-)
To view, visit change 2658378. To unsubscribe, or for help writing mail filters, visit settings.
Ryan Keane (Gerrit)
Attention is currently required from: Yuchen Liu.
Patch set 1:Commit-Queue +1
1 comment:
Patchset:
The previous change got reverted due to a buildbot running on a non-debug build. This CL should fix that
To view, visit change 2658378. To unsubscribe, or for help writing mail filters, visit settings.
Yuchen Liu (Gerrit)
Attention is currently required from: Ryan Keane.
Patch set 1:Code-Review +1
Ryan Keane (Gerrit)
Attention is currently required from: Ryan Keane.
Patch set 1:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
[Cast] Add BufferIdManager
This CL introduces the concept of a "BufferId", or a unique ID assigned
to each PushBuffer call. Additionally, it introduces the BufferIdManager
class, a utility class used to approximate these IDs at a given time.
This functionality is required for audio-audio sync as defined in:
go/cast-media-chromium-runtime
This CL un-reverts CL:
https://chromium-review.googlesource.com/c/chromium/src/+/2580782
The issue was that a buildbot runs the unit tests on a non-debug build,
so a DCHECK doesn't fail the bot run.
Bug: internal b/171495618
Change-Id: Iaeb529f0a11ce57264e47e6e9ae4392bf9ad15b0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2658378
Reviewed-by: Yuchen Liu <yuc...@chromium.org>
Commit-Queue: Ryan Keane <rwk...@google.com>
Cr-Commit-Position: refs/heads/master@{#849225}
---
M chromecast/media/cma/BUILD.gn
M chromecast/media/cma/backend/proxy/BUILD.gn
A chromecast/media/cma/backend/proxy/buffer_id_manager.cc
A chromecast/media/cma/backend/proxy/buffer_id_manager.h
A chromecast/media/cma/backend/proxy/buffer_id_manager_unittest.cc
5 files changed, 322 insertions(+), 0 deletions(-)