[Media] Add Symphonia support to AudioFileReader [chromium/src : main]
Jordan Bayles (Gerrit)
Jordan Bayles voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Dale Curtis
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jordan Bayles (Gerrit)
Jordan Bayles added 1 comment![]( "Open in Gerrit")
Eugene, mind taking a look if you are around today or tomorrow?
Related details
- Dale Curtis
- Eugene Zemtsov
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eugene Zemtsov (Gerrit)
Eugene Zemtsov added 1 comment![]( "Open in Gerrit")
// audio streams. All public methods and callbacks are trampolined to the
// |task_runner_| so that no locks are required for thread safety.I don't think this comment is accurate, because `BindCallbackIfNeeded` uses the current task runner and not the one provided via the constructor.
Related details
- Dale Curtis
- Jordan Bayles
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jordan Bayles (Gerrit)
Jordan Bayles voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jordan Bayles
| Commit-Queue | +1 |
1 comment
// audio streams. All public methods and callbacks are trampolined to the
// |task_runner_| so that no locks are required for thread safety.I don't think this comment is accurate, because `BindCallbackIfNeeded` uses the current task runner and not the one provided via the constructor.
- Dale Curtis
- Eugene Zemtsov
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eugene Zemtsov (Gerrit)
Eugene Zemtsov added 1 comment![]( "Open in Gerrit")
task_runner_->PostTask(FROM_HERE, std::move(closure));here we use `task_runner`, but in all other places we just post to the current sequence. Don't we want to be consistent with the callbacks?
Related details
- Dale Curtis
- Jordan Bayles
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jordan Bayles (Gerrit)
Jordan Bayles added 1 comment![]( "Open in Gerrit")
task_runner_->PostTask(FROM_HERE, std::move(closure));here we use `task_runner`, but in all other places we just post to the current sequence. Don't we want to be consistent with the callbacks?
Oh yes. This matches legacy behavior. I want to fix this up, just in a separate patch.
I think all of the callbacks should be posted to the task runner. It's the media task runner. The decoder factory assumes that all decoders are single threaded and should run on the media task runner -- which is the task runner passed to these classes.
This cleanup should apply to both the Symphonia and FFmpeg decoders, and potentially others.
Related details
- Dale Curtis
- Eugene Zemtsov
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis added 2 comments![]( "Open in Gerrit")
enum class ExecutionMode { kAsynchronous, kSynchronous };I don't think this should be on AudioDecoder. it's a bit too specialized and specific to the impl.
TEST_P(AudioFileReaderTest, FLAC) {Don't nest in prop codecs section
Related details
- Eugene Zemtsov
- Jordan Bayles
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jordan Bayles (Gerrit)
Jordan Bayles voted and added 2 comments![]( "Open in Gerrit")
Votes added by Jordan Bayles
| Commit-Queue | +1 |
2 comments
enum class ExecutionMode { kAsynchronous, kSynchronous };I don't think this should be on AudioDecoder. it's a bit too specialized and specific to the impl.
Sounds good. I also feel like this could be a utility class of some kind because a lot of classes do a callback interface and the option to post back or not is desired. I'll give it some thought, and just have it duplicated on the two decoders for now.
Don't nest in prop codecs section
- Dale Curtis
- Eugene Zemtsov
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis voted and added 4 comments![]( "Open in Gerrit")
Votes added by Dale Curtis
| Code-Review | +1 |
4 comments
#if BUILDFLAG(ENABLE_SYMPHONIA)Is this one needed? Or does it happy do nothing if you drop it. It's always nice to reduce buildflag spam where possible.
raw_ptr<MediaLog> media_log_;const this or nullptr initialize.
scoped_refptr<base::SequencedTaskRunner> task_runner_;const
? base::BindPostTaskToCurrentDefault(std::forward<T>(callback))FYI, there's a `BindPostTask(task_runner,` variant. Lets use that here instead of adding a new default dependency.
Related details
- Eugene Zemtsov
- Jordan Bayles
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eugene Zemtsov (Gerrit)
Eugene Zemtsov voted Code-Review+1![]( "Open in Gerrit")
- Jordan Bayles
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jordan Bayles (Gerrit)
Jordan Bayles voted and added 4 comments![]( "Open in Gerrit")
Votes added by Jordan Bayles
| Commit-Queue | +2 |
4 comments
Is this one needed? Or does it happy do nothing if you drop it. It's always nice to reduce buildflag spam where possible.
Not needed.
const this or nullptr initialize.
SGTM.
scoped_refptr<base::SequencedTaskRunner> task_runner_;Jordan Baylesconst
Done
? base::BindPostTaskToCurrentDefault(std::forward<T>(callback))FYI, there's a `BindPostTask(task_runner,` variant. Lets use that here instead of adding a new default dependency.
Agreed. I'm pretty sure, based on my understanding of this class and the FFmpegAudioDecoder, as well as the decoder factory, that in this context "default" should be the same as task_runner -- otherwise we really need to update the comment here:
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dale Curtis
| Code-Review | +1 |
1 comment
? base::BindPostTaskToCurrentDefault(std::forward<T>(callback))Jordan BaylesFYI, there's a `BindPostTask(task_runner,` variant. Lets use that here instead of adding a new default dependency.
Agreed. I'm pretty sure, based on my understanding of this class and the FFmpegAudioDecoder, as well as the decoder factory, that in this context "default" should be the same as task_runner -- otherwise we really need to update the comment here:
- Jordan Bayles
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jordan Bayles (Gerrit)
Jordan Bayles voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[Media] Add Symphonia support to AudioFileReader
This patch adds support for using the SymphoniaAudioReader
in the AudioFileReader class, if build time support is enabled,
for supported codecs. Test coverage is also included.
- M media/base/audio_decoder.cc
- M media/filters/audio_file_reader.cc
- M media/filters/audio_file_reader.h
- M media/filters/audio_file_reader_unittest.cc
- M media/filters/ffmpeg_audio_decoder.h
- M media/filters/symphonia_audio_decoder.cc
- M media/filters/symphonia_audio_decoder.h
Code-Review: +1 by Dale Curtis, +1 by Eugene Zemtsov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |