Add MF AAC Encoder. [chromium/src : main]
Austin Orion (Gerrit)
Attention is currently required from: Eugene Zemtsov.
Austin Orion would like Eugene Zemtsov to review this change.
Add MF AAC Encoder.
This CL introduces MFAudioEncoder which uses the Windows Media
Foundation's AAC encoder.
https://docs.microsoft.com/en-us/windows/win32/medfound/aac-encoder
The MF AAC encoder was designed to encode files, where all the data
is available at the start, and so the API is not perfectly suited
for stream encoding. Specifically, there is no callback or event
to let us know when output data is ready, or when it is ready for more
input.
We make this work with the EncodeAsync function that runs whenever we get
input data, the function is basically:
1. Give the input data to the encoder, if it will accept it.
2. Process any output data that is ready.
3. If we were unable to give the input data in step 1, go back to step 1.
This loop may spin for some time, as the encoder won't accept new input
until it has finished processing enough of the previous input that there
is room for more. This loop is loosely based on the MFT Processing model:
https://docs.microsoft.com/en-us/windows/win32/medfound/basic-mft-processing-model#process-data
This design assumes that consumers of MFAudioEncoder will regularly
provide more input data, as we don't check for output unless we have
received new input. We could easily add a timer that will check for
output, and resets whenever we get new input, if this is a concern.
This will be used by MojoAudioEncoder to enable encoding to be done
in the GPU process when using WebCodecs and other consumers. See these
other related CLs:
3243816: webcodecs: Mojo interface, service and client for AudioEncoder | https://chromium-review.googlesource.com/c/chromium/src/+/3243816
3308655: webcodecs: Binding for Mojo Audio Encoder | https://chromium-review.googlesource.com/c/chromium/src/+/3308655
Bug: 1072056
Change-Id: I89b6a21407e5d7ff24a09569b34ffbdd8073ad67
---
A media/gpu/windows/mf_audio_encoder.h
M media/gpu/BUILD.gn
A media/gpu/windows/mf_audio_encoder.cc
M media/base/audio_parameters.cc
M media/base/audio_parameters.h
M content/browser/media/media_internals.cc
6 files changed, 649 insertions(+), 0 deletions(-)
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Austin Orion (Gerrit)
Attention is currently required from: Eugene Zemtsov.
1 comment:
Patchset:
Hey Eugene and edgecapabilitiesdev,
The MF AAC encoder is ready for a preliminary review. I still have some work to do, such as adding tests, loading the .dlls (for N SKUs), and implementing PrepareExtraData, but PTAL at what is here so far.
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Rahul Singh (Gerrit)
Attention is currently required from: Eugene Zemtsov, Austin Orion.
9 comments:
Patchset:
Thanks for working on this Austin! Here are some early comments. I'll take another look tomorrow.
File media/gpu/windows/mf_audio_encoder.h:
2022. Here and in the .cc file
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #3, Line 31:
#include <winerror.h>nit: Should this be integrated with the second group of headers above?
Patch Set #3, Line 178:
// anonymous namespacesuper nit: To denote anonymous namespace, I've usually seen "// namespace" which matches the "namespace {" declaration at the top.
Patch Set #3, Line 234:
kEncoderUnsupportedConfigInstead of reusing StatusCodes, would it be helpful to add new and more descriptive entries to EncoderStatusTraits?
https://source.chromium.org/chromium/chromium/src/+/main:media/base/encoder_status.h;drc=8cc47916d332c54ef41b67edb4a20c109dd884af;l=12Something like kEncoderUnsupportedBitrate, kEncoderUnsupportedChannels, kEncoderUnsupportedSampleRate, etc.
Same for kEncoderInitializationError usage below.Patch Set #3, Line 241:
AUDIO_PCM_LOW_LATENCYCan we add a comment explaining why this particular format was picked?
Patch Set #3, Line 252:
CoCreateInstancePer this comment in the same directory as this file, we want to avoid calling CoCreateInstance as it requires a COM apartment to be initialized. We want to avoid initializing COM on the GPU main thread.
https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/windows/dxva_video_decode_accelerator_win.cc;drc=689e7aa6b94febf3f2c8c169768a1769132098b3;l=296
Are we on the GPU main thread here? If so, can we use MFTEnumEx instead?
https://docs.microsoft.com/en-us/windows/win32/api/mfapi/nf-mfapi-mftenumex#examples// Since there is only one input and one output stream, both will have an ID
// of 0.Should we include a link to this remark that states that setting input and output stream id to 0 is the convention when we have one of each?
https://docs.microsoft.com/en-us/windows/win32/api/mftransform/nf-mftransform-imftransform-getstreamids#remarks// Each output sample should contain a complete, unbroken frame of audio
// data.
DCHECK(output_stream_info.dwFlags | MFT_OUTPUT_STREAM_WHOLE_SAMPLES);
// Each output sample should contain exactly one frame of audio data.
DCHECK(output_stream_info.dwFlags |
MFT_OUTPUT_STREAM_SINGLE_SAMPLE_PER_BUFFER);
// All output samples are the same size.
DCHECK(output_stream_info.dwFlags | MFT_OUTPUT_STREAM_FIXED_SAMPLE_SIZE);
// We are required to process the output for this stream (so that our
// inputs aren't discarded).
DCHECK(!output_stream_info.dwFlags | MFT_OUTPUT_STREAM_LAZY_READ);
// This stream should not be removable.
DCHECK(!output_stream_info.dwFlags | MFT_OUTPUT_STREAM_REMOVABLE);
// The encoder should not discard any output data.
DCHECK(!output_stream_info.dwFlags | MFT_OUTPUT_STREAM_DISCARDABLE);
// This output stream is required.
DCHECK(!output_stream_info.dwFlags | MFT_OUTPUT_STREAM_OPTIONAL);
// We should be responsible for allocating the output samples.
DCHECK(!output_stream_info.dwFlags | MFT_OUTPUT_STREAM_PROVIDES_SAMPLES);
DCHECK(!output_stream_info.dwFlags | MFT_OUTPUT_STREAM_CAN_PROVIDE_SAMPLES);To reduce the length of this method, could we move these asserts into a helper?
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Austin Orion (Gerrit)
Attention is currently required from: Eugene Zemtsov, Rahul Singh.
8 comments:
File media/gpu/windows/mf_audio_encoder.h:
2022. Here and in the . […]
Done
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #3, Line 31:
#include <winerror.h>nit: Should this be integrated with the second group of headers above?
This is a duplicate anyways and needs to be removed, along with several other includes. Thanks for reminding me!
Patch Set #3, Line 178:
// anonymous namespace
super nit: To denote anonymous namespace, I've usually seen "// namespace" which matches the "namesp […]
Done
Patch Set #3, Line 234:
kEncoderUnsupportedConfig
Instead of reusing StatusCodes, would it be helpful to add new and more descriptive entries to Encod […]
I think we can add detail by providing messages, but it would be useful to have a more detailed enum so we can log it to UMA. I'll look into this.
Patch Set #3, Line 241:
AUDIO_PCM_LOW_LATENCYCan we add a comment explaining why this particular format was picked?
We can actually just remove this, the only value we need to store is the number of channels.
But to your question, there is only one valid input type and that is PCM audio samples. I used PCM_LOW_LATENCY because that indicates that we'd like to receive the samples with low latency, but like I said it affects nothing because we don't use it.
Patch Set #3, Line 252:
CoCreateInstance
Per this comment in the same directory as this file, we want to avoid calling CoCreateInstance as it […]
We are on the GPU main thread, I'll look into this.
// Since there is only one input and one output stream, both will have an ID
// of 0.
Should we include a link to this remark that states that setting input and output stream id to 0 is […]
Done
// Each output sample should contain a complete, unbroken frame of audio
// data.
DCHECK(output_stream_info.dwFlags | MFT_OUTPUT_STREAM_WHOLE_SAMPLES);
// Each output sample should contain exactly one frame of audio data.
DCHECK(output_stream_info.dwFlags |
MFT_OUTPUT_STREAM_SINGLE_SAMPLE_PER_BUFFER);
// All output samples are the same size.
DCHECK(output_stream_info.dwFlags | MFT_OUTPUT_STREAM_FIXED_SAMPLE_SIZE);
// We are required to process the output for this stream (so that our
// inputs aren't discarded).
DCHECK(!output_stream_info.dwFlags | MFT_OUTPUT_STREAM_LAZY_READ);
// This stream should not be removable.
DCHECK(!output_stream_info.dwFlags | MFT_OUTPUT_STREAM_REMOVABLE);
// The encoder should not discard any output data.
DCHECK(!output_stream_info.dwFlags | MFT_OUTPUT_STREAM_DISCARDABLE);
// This output stream is required.
DCHECK(!output_stream_info.dwFlags | MFT_OUTPUT_STREAM_OPTIONAL);
// We should be responsible for allocating the output samples.
DCHECK(!output_stream_info.dwFlags | MFT_OUTPUT_STREAM_PROVIDES_SAMPLES);
DCHECK(!output_stream_info.dwFlags | MFT_OUTPUT_STREAM_CAN_PROVIDE_SAMPLES);To reduce the length of this method, could we move these asserts into a helper?
Done
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Eugene Zemtsov (Gerrit)
Attention is currently required from: Austin Orion, Rahul Singh.
2 comments:
File media/base/audio_parameters.h:
Patch Set #3, Line 134:
AUDIO_BITSTREAM_AACWe don't need to add each compression format here. This is only for audio formats that are sent to playback devices.
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #3, Line 244:
AUDIO_BITSTREAM_AACPlease use AUDIO_PCM_LOW_LATENCY, this describes parameters of audio segment when the buffer is decoded. If encoder doesn't change channel layout or sample rate, there is not reason to have separate output_params_.
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Steve Becker (Gerrit)
Attention is currently required from: Austin Orion, Rahul Singh.
21 comments:
File media/gpu/BUILD.gn:
Patch Set #4, Line 239:
"wmcodecdspuuid.lib",Should we also delay load the corresponding DLL for this lib, wmvdspa.dll?
File media/gpu/windows/mf_audio_encoder.h:
#include "base/task/task_traits.h"
#include "base/task/thread_pool.h"
#include "base/timer/timer.h"Please remove unused headers and forward declare when possible.
Patch Set #3, Line 34:
class MEDIA_GPU_EXPORT MFAudioEncoder : public AudioEncoder {Please consider adding a comment banner to explain this class. The change description had a lot of great information that we could add in a comment.
Patch Set #3, Line 56:
= nullptr;nit: ComPtr has a default constructor that initializes to nullptr.
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #3, Line 46:
constexpr int kDefaultBitrate = 96000;Should we add a comment that explains how we chose this value?
Patch Set #3, Line 49:
IMFMediaType*Need to use ComPtr to prevent memory leaks.
Patch Set #3, Line 80:
IMFMediaTypeSame as above, we need to use a ComPtr to prevent memory leaks.
Patch Set #3, Line 144:
DCHECK_GEShould we make this a CHECK to ensure we don't have any out of bound writes?
Patch Set #3, Line 252:
CoCreateInstance
We are on the GPU main thread, I'll look into this.
I think initializing the Media Foundation, which we do above, will also initialize COM on this thread. There's also the CreateCOMSTATaskRunner(), which might be helpful for us to use to isolate COM calls to a single task runner/thread.
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #4, Line 133:
std::unique_ptr<AudioBus> audio_bus,nit: Should |audio_bus| be const reference to make it clear that it's an input parameter?
Patch Set #4, Line 133:
PrepareInputSampleShould we give this function a more descriptive name? Can we describe what "Prepare" involves? Maybe something like CreateMFSampleFromAudioBus()?
Patch Set #4, Line 141:
= nullptrnit: ComPtr provides a default constructor.
Patch Set #4, Line 149:
buffernit: Since there are a lot of different buffer variables involved, it might be helpful to differentiate between source buffers and destination buffers using variable names involving source/destination.
Patch Set #4, Line 175:
= nullptrnit: ComPtr provides a default constructor.
super nit: prefer 0 for flags over NULL to avoid macros. NULL is a macro for 0.
Patch Set #4, Line 303:
/*stream_id=*/0nit: Should we use the |stream_id| constant that we defined above?
Patch Set #4, Line 307:
VerifyOutputStreamFlagsnit: it's a little strange that if verification fails, we do not handle an error. Should we rename VerifyOutputStreamFlags() to make it clearer that it uses DCHECK verification only?
Patch Set #4, Line 343:
- base::TimeTicks()Why do we subtract base:TimeTicks(), which is initialized to 0? Is there a cleaner why to produce a TimeDelta?
Patch Set #4, Line 363:
weak_factory_.GetWeakPtr(),weak_ptr is not thread safe. It looks like we might be using this MFAudioEncoder object across multiple sequences/threads. Do we need to make MFAudioEncoder refcounted to do this safely?
Also, this reference might be helpful:
https://chromium.googlesource.com/chromium/src/+/refs/heads/main/docs/threading_and_tasks.md
In particular, what keeps MFAudioEncoder alive while the processing
Patch Set #4, Line 405:
not_acceptingDo we need a yield or a sleep to prevent this loop from spinning and consuming 100% of the CPU when the encoder is not accepting new input?
Maybe we should make a global constant for all stream ID usage in this file?
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Steve Becker, Austin Orion, Rahul Singh.
13 comments:
File media/base/audio_parameters.cc:
Patch Set #4, Line 23:
case AudioParameters::AUDIO_BITSTREAM_AAC:Shouldn't be necessary to add this unless you plan to add bitstream AAC output. Just use PCM linear.
File media/gpu/BUILD.gn:
Patch Set #4, Line 239:
"wmcodecdspuuid.lib",
Should we also delay load the corresponding DLL for this lib, wmvdspa. […]
Yes please do. Please ensure this gracefully fails on Windows N too.
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #3, Line 49:
IMFMediaType*
Need to use ComPtr to prevent memory leaks.
Also, nullptr is the style these days, here and elsewhere.
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #4, Line 47:
if (FAILED(hr))https://source.chromium.org/chromium/chromium/src/+/main:media/base/win/mf_helpers.h;l=51;drc=74ce33d27ee2ca0db7e8fdff08ff71cd86eb35e2 would clean this up a lot and add useful logging.
Patch Set #4, Line 144:
HRESULT hr = MFCreateMemoryBuffer(input_buffer_size, &input_media_buffer);Is there a pooling mechanism we should use for these? Or can we just create once and reuse over and over?
Patch Set #4, Line 148:
// Fill `input_media_buffer` with data from `audio_bus`.Allocate a temp buffer at the class level instead of reallocating this every sample.
Patch Set #4, Line 150:
audio_bus->ToInterleaved<media::SignedInt16SampleTypeTraits>(Is it not possible to provide planar float for encoding? That way you can potentially avoid a copy/transform.
Patch Set #4, Line 199:
: task_runner_(base::ThreadPool::CreateSingleThreadTaskRunner({})) {}It's preferable to have the task runner passed in instead so tests can manage the thread on their own. Does audio encoding actually need a thread though?
Patch Set #4, Line 225:
if (base::win::GetVersion() >= base::win::Version::WIN10) {Document why.
Patch Set #4, Line 235:
if (options.sample_rate != 44100 && options.sample_rate != 48000) {Document why. 96khZ AAC is a thing.
Patch Set #4, Line 276:
IMFMediaType* input_media_type =raw ptr seems a bit concerning here. Should this be ComPtr? ditto below?
Patch Set #4, Line 307:
VerifyOutputStreamFlags
nit: it's a little strange that if verification fails, we do not handle an error. […]
Since that function only DCHECKs it's probably more legible as
DCHECK(VerifyOutputStremFlags(...)) and having that function return a boolean instead of N dchecks.
Patch Set #4, Line 420:
hr = MFCreateMemoryBuffer(output_stream_info.cbSize, &encoded_buffer);Similar to above, do we need to recreate a new buffer everytime?
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Austin Orion (Gerrit)
Attention is currently required from: Austin Orion.
22 comments:
Patchset:
Still working on the more complex comments, but I've addressed the simpler ones.
File media/base/audio_parameters.h:
Patch Set #3, Line 134:
AUDIO_BITSTREAM_AAC
We don't need to add each compression format here. […]
Done
File media/base/audio_parameters.cc:
Patch Set #4, Line 23:
case AudioParameters::AUDIO_BITSTREAM_AAC:Shouldn't be necessary to add this unless you plan to add bitstream AAC output. Just use PCM linear.
Done
File media/gpu/windows/mf_audio_encoder.h:
#include "base/task/task_traits.h"
#include "base/task/thread_pool.h"
#include "base/timer/timer.h"Please remove unused headers and forward declare when possible.
Done
Patch Set #3, Line 34:
class MEDIA_GPU_EXPORT MFAudioEncoder : public AudioEncoder {
Please consider adding a comment banner to explain this class. […]
Done. Let me know if there is any other information you think may be helpful to include here.
Patch Set #3, Line 56:
= nullptr;nit: ComPtr has a default constructor that initializes to nullptr.
Done
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #3, Line 46:
constexpr int kDefaultBitrate = 96000;
Should we add a comment that explains how we chose this value?
Done
Patch Set #3, Line 49:
IMFMediaType*
Also, nullptr is the style these days, here and elsewhere.
Done and done.
Patch Set #3, Line 80:
IMFMediaType
Same as above, we need to use a ComPtr to prevent memory leaks.
Done
Patch Set #3, Line 144:
DCHECK_GEShould we make this a CHECK to ensure we don't have any out of bound writes?
Done
Patch Set #3, Line 244:
AUDIO_BITSTREAM_AAC
Please use AUDIO_PCM_LOW_LATENCY, this describes parameters of audio segment when the buffer is deco […]
Done
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #4, Line 47:
if (FAILED(hr))
https://source.chromium.org/chromium/chromium/src/+/main:media/base/win/mf_helpers. […]
Great suggestion! Done.
Patch Set #4, Line 133:
PrepareInputSample
Should we give this function a more descriptive name? Can we describe what "Prepare" involves? May […]
Done
Patch Set #4, Line 133:
std::unique_ptr<AudioBus> audio_bus,nit: Should |audio_bus| be const reference to make it clear that it's an input parameter?
Done
Patch Set #4, Line 141:
= nullptrnit: ComPtr provides a default constructor.
Done
Patch Set #4, Line 175:
= nullptrnit: ComPtr provides a default constructor.
Done
Patch Set #4, Line 276:
IMFMediaType* input_media_type =
raw ptr seems a bit concerning here. […]
Done
super nit: prefer 0 for flags over NULL to avoid macros. NULL is a macro for 0.
Done
Patch Set #4, Line 303:
/*stream_id=*/0nit: Should we use the |stream_id| constant that we defined above?
Done
Patch Set #4, Line 307:
VerifyOutputStreamFlags
Since that function only DCHECKs it's probably more legible as […]
Good suggestion! Done.
Patch Set #4, Line 343:
- base::TimeTicks()
Why do we subtract base:TimeTicks(), which is initialized to 0? Is there a cleaner why to produce a […]
You've got it, we do this to convert from TimeTicks to TimeDelta. I do not think there is a cleaner way to do this. The alternative would be to use To/FromInternalValue, but these are on their way out:
https://crbug.com/634507It seems that using the arithmetic operators is recommended:
// ...For deserializing TimeTicks values,
// prefer TimeTicks + TimeDelta();
https://source.chromium.org/chromium/chromium/src/+/main:base/time/time.h;l=1099This is the pattern that is used in the Opus encoder as well:
https://source.chromium.org/chromium/chromium/src/+/main:media/audio/audio_opus_encoder.cc;l=228?q=audio_opus_encoder&ss=chromium%2Fchromium%2Fsrc
Maybe we should make a global constant for all stream ID usage in this file?
Done
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Austin Orion (Gerrit)
Attention is currently required from: Steve Becker.
1 comment:
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #4, Line 363:
weak_factory_.GetWeakPtr(),weak_ptr is not thread safe. […]
I see the problem. What if we make EncodeAsync and ProcessOutput anonymous functions, and passed in encoder_ and output_cb_? Then they can run on another thread and not care if MFAudioEncoder is alive or not.
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Sunggook Chue (Gerrit)
Attention is currently required from: Steve Becker, Austin Orion.
14 comments:
Patchset:
adds minors comments.
File media/gpu/windows/mf_audio_encoder.cc:
ChannelLayout channel_layout;
switch (options.channels) {
case 1:
channel_layout = CHANNEL_LAYOUT_MONO;
break;
case 2:
channel_layout = CHANNEL_LAYOUT_STEREO;
break;
case 6:
if (::base::win::GetVersion() >= ::base::win::Version::WIN10) {
channel_layout = CHANNEL_LAYOUT_5_1;
break;
}
[[fallthrough]];
default:
std::move(done_cb).Run(EncoderStatus::Codes::kEncoderUnsupportedConfig);
return;
}minor: we can move this one to right above channel_layout is referenced (line 285 or 286).
Patch Set #5, Line 267:
44100 && options.sample_rate != 48000Are these numbers (44100, 48000) generic knowledge? otherwise, we can replace it use concrete named macro?
timestamp_tracker_ =
std::make_unique<AudioTimestampHelper>(options.sample_rate);minor: can it also move to before setting initialized_ = true?
if (FAILED(hr)) {
std::move(done_cb).Run(EncoderStatus::Codes::kEncoderInitializationError);
return;
}This pattern repeate quite a lot,
How about creating macro that (mf_audio_decoder.cc uses macro as well).
CALL_CALLBACK__ON_HR_FAILURE(CoCreateInstance(CLSID_AACMFTEncoder, ..), EncoderStatus::Codes::kEncoderInitializationError);
or
CALL_CALLBACK__ON_HR_FAILURE(hr, EncoderStatus::Codes::kEncoderInitializationError, "log error message"); It is following pattern of mf_audio_decoder.cc).
such as CALL_CALLBACK_ON_HR_FAILURE(hr, EncoderStatus::Codes::kEncoderInitializationError).
btw, why we swallow the system error of hr and instead custom's?
Patch Set #5, Line 320:
output_media_typeany reason not using SUCCEEDED(hr)?
if (!initialized_) {
std::move(done_cb).Run(
EncoderStatus::Codes::kEncoderInitializeNeverCompleted);
return;
}this should be before above DCHECK because these variables (channel_count_, etc) must be false if initialized_ is false.
Patch Set #5, Line 410:
not_accepting = true;How about output processing doesn't handle data, then doesn't ProcessInput return MF_E_NOTACCEPTING all the time?
I'm not much familiar with these API usage, so my question might not be relevant.
Patch Set #5, Line 448:
= nullptrno need, same for every ComPtr init in this file.
no need to set for out parameters when its return value have success/failure like HRESULT.
if (FAILED(hr))
return hr;it looks like we can use LOG_HR_ON_FAILURE like mf_audio_decoder.cc?
Patch Set #5, Line 480:
return E_FAIL;E_FAIL is very useful, but at the same time it is very annoying from the caller perspective; can't figure out why it failed.
Can we have any more specific error? (you can search ULONG based win32 error (winerror.h) and use HRESULT_FROM_WIN32) if it is okay in this file.
Patch Set #5, Line 528:
if (!initialized_) {same here, we need to check initialized_ before accessing member variables (DCHECK above).
Patch Set #5, Line 539:
MF_E_TRANSFORM_NEED_MORE_INPUT"It will stop calling ProcessOutput() once it returns MF_E_TRANSFORM_NEED_MORE_INPUT." from "%SDXROOT%\avcore\codecdsp\audio\AC3Enc\Current\MFT\DDEncMFT.cpp". not sure if it is about CLSID_AACMFTEncoder.
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Rahul Singh (Gerrit)
Attention is currently required from: Austin Orion.
9 comments:
File media/gpu/windows/mf_audio_encoder.h:
Patch Set #4, Line 47:
encoder_Should we rename this to mf_encoder_ which would convey it'll be used to call Windows MediaFoundation APIs?
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #3, Line 137:
backing_input_bufferCould we name this `input_media_buffer_pointer` or something similar to convey that it's derived from the `input_media_buffer`?
Patch Set #3, Line 252:
CoCreateInstance
I think initializing the Media Foundation, which we do above, will also initialize COM on this threa […]
Turns out COM is initialized on GPU process start up with MTA.
https://source.chromium.org/chromium/chromium/src/+/main:content/gpu/gpu_main.cc;drc=7a6b3e61d8a857b0b2745d936123ef77a2cb15e2;l=228This means all threads that we subsequently create are also placed in the MTA. Source:
https://devblogs.microsoft.com/oldnewthing/20130419-00/?p=4613#:~:text=A%20thread%20has%20an%20implicit%20MTA%20apartment%20type,from%20other%20threads%20and%20is%20not%20initialized%20directly.
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #4, Line 405:
} while (not_accepting);Not sure I understand outer while termination here. Do we keep going till ProcessInput returns an error HRESULT that isn't MF_E_NOTACCEPTING and then we return?
Should we use 0u?
Patch Set #4, Line 485:
TimeTicksIs the addition operation helping us convert to base::TimeTicks() here? If so, should we add a comment to clarify this?
Patch Set #4, Line 506:
MFT_MESSAGE_COMMAND_DRAINPer this documentation:
https://docs.microsoft.com/en-us/windows/win32/medfound/basic-mft-processing-model
it doesn't look like we need to call MFT_MESSAGE_COMMAND_FLUSH as part of the processing flow.In this code, it appears we send an MFT_MESSAGE_COMMAND_DRAIN message and then ProcessOutput() till we receive hr == MF_E_TRANSFORM_NEED_MORE_INPUT.
Can the MFTransform return that HRESULT while it's still holding on to input? If not, can we get rid of the MFT_MESSAGE_COMMAND_FLUSH message here?
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #5, Line 267:
44100 && options.sample_rate != 48000Are these numbers (44100, 48000) generic knowledge? otherwise, we can replace it use concrete named […]
Good catch Sunggook. Austin, should we include these values in the constexpr list at the top of this file? (The style guide recommends avoiding Macros when possible).
https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros
if (FAILED(hr)) {
std::move(done_cb).Run(EncoderStatus::Codes::kEncoderInitializationError);
return;
}
This pattern repeate quite a lot, […]
Good idea to pull out the repeating code. Per the style guide, we should prefer a helper function instead of a macro for this.
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Austin Orion (Gerrit)
Attention is currently required from: Dale Curtis, Eugene Zemtsov, Steve Becker, Sunggook Chue, Rahul Singh.
32 comments:
Patchset:
Sorry for the delay, a new iteration has been pushed. This was a significant refactor, and I am still working on updating the CL description, as well as adding some tests.
File media/gpu/BUILD.gn:
Patch Set #4, Line 239:
"wmcodecdspuuid.lib",
Yes please do. Please ensure this gracefully fails on Windows N too.
We can remove the dependency on wmcodecdspuuid.lib by using IID_PPV_ARGS instead, so this was removed. I did add a delay load for mfaacenc.dll and a PreSandboxInitialization function, as well as a class factory to create the encoder so that we can gracefullly fail when it is not available.
File media/gpu/windows/mf_audio_encoder.h:
Patch Set #4, Line 47:
encoder_
Should we rename this to mf_encoder_ which would convey it'll be used to call Windows MediaFoundatio […]
Done
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #3, Line 137:
backing_input_buffer
Could we name this `input_media_buffer_pointer` or something similar to convey that it's derived fro […]
Done
Patch Set #3, Line 252:
CoCreateInstance
Turns out COM is initialized on GPU process start up with MTA. […]
Ended up using a ClassFactory here to initialize the encoder.
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #4, Line 144:
HRESULT hr = MFCreateMemoryBuffer(input_buffer_size, &input_media_buffer);Is there a pooling mechanism we should use for these? Or can we just create once and reuse over and […]
We can't for the input buffers because we have no way of knowing when the encoder has released the sample we provide:
"Do not re-use the sample until the MFT releases the sample."
https://docs.microsoft.com/en-us/windows/win32/api/mftransform/nf-mftransform-imftransform-processinput#remarksWe are able to do this for the output buffer, and I've done so.
Patch Set #4, Line 148:
// Fill `input_media_buffer` with data from `audio_bus`.Allocate a temp buffer at the class level instead of reallocating this every sample.
Good idea. It's convenient this is a vector so we can resize it if the input data is too large. Done.
Patch Set #4, Line 149:
buffer
nit: Since there are a lot of different buffer variables involved, it might be helpful to differenti […]
Done
Patch Set #4, Line 150:
audio_bus->ToInterleaved<media::SignedInt16SampleTypeTraits>(Is it not possible to provide planar float for encoding? That way you can potentially avoid a copy/t […]
Unfortunately no, the MF AAC encoder requires it's input data to be 16 bits per sample
"Bits per sample. Must be 16."
https://docs.microsoft.com/en-us/windows/win32/medfound/aac-encoder#input-types
Patch Set #4, Line 199:
: task_runner_(base::ThreadPool::CreateSingleThreadTaskRunner({})) {}
It's preferable to have the task runner passed in instead so tests can manage the thread on their ow […]
I've updated this to use a SequencedTaskRunner, and I've provided a constructor overload that accepts a task_runner.
Patch Set #4, Line 225:
if (base::win::GetVersion() >= base::win::Version::WIN10) {Document why.
Done
Patch Set #4, Line 235:
if (options.sample_rate != 44100 && options.sample_rate != 48000) {
Document why. 96khZ AAC is a thing.
These are the only supported sample rates for the MF AAC encoder. I put these into a constexpr at the top, just below a link to the docs.
Patch Set #4, Line 363:
weak_factory_.GetWeakPtr(),
I see the problem. […]
This is done. And to answer your question about what keeps MFAudioEncoder alive, see Eugene's CL:
https://chromium-review.googlesource.com/c/chromium/src/+/3308655Specifically, web codecs holds a
std::unique_ptr<MediaEncoderType> media_encoder_;
declared in
third_party/blink/renderer/modules/webcodecs/encoder_base.hand initialized via this stack:
GpuMojoMediaClient::CreateAudioEncoder (gpu_mojo_media_client.cc)
InterfaceFactoryImpl::CreateAudioEncoder (mojo_media_client.cc)
CreatePlatformAudioEncoder (web_codecs/audio_encoder.cc)
AudioEncoder::CreateMediaAudioEncoder (web_codecs/audio_encoder.cc)
Patch Set #4, Line 405:
not_accepting
Do we need a yield or a sleep to prevent this loop from spinning and consuming 100% of the CPU when […]
This has been redesigned to post another task to the task_runner_, if necessary, instead of looping.
Patch Set #4, Line 405:
} while (not_accepting);
Not sure I understand outer while termination here. […]
This should be clearer with the new design.
Patch Set #4, Line 420:
hr = MFCreateMemoryBuffer(output_stream_info.cbSize, &encoded_buffer);Similar to above, do we need to recreate a new buffer everytime?
No, we can reuse this buffer. This is done.
Should we use 0u?
Done
Patch Set #4, Line 485:
TimeTicks
Is the addition operation helping us convert to base::TimeTicks() here? If so, should we add a comme […]
Done
Patch Set #4, Line 506:
MFT_MESSAGE_COMMAND_DRAIN
Per this documentation: […]
Flush is not a required part of the processing flow, but chromium's AudioEncoder interface requires that we implement it.
And yes, the encoder can require more input while still holding on to some, if it has fewer than `kSamplesPerFrame` (1024) samples. We must flush these before the encoder will accept input again.
File media/gpu/windows/mf_audio_encoder.cc:
ChannelLayout channel_layout;
switch (options.channels) {
case 1:
channel_layout = CHANNEL_LAYOUT_MONO;
break;
case 2:
channel_layout = CHANNEL_LAYOUT_STEREO;
break;
case 6:
if (::base::win::GetVersion() >= ::base::win::Version::WIN10) {
channel_layout = CHANNEL_LAYOUT_5_1;
break;
}
[[fallthrough]];
default:
std::move(done_cb).Run(EncoderStatus::Codes::kEncoderUnsupportedConfig);
return;
}minor: we can move this one to right above channel_layout is referenced (line 285 or 286).
I did some refactoring and this ended up in a helper function.
Patch Set #5, Line 267:
44100 && options.sample_rate != 48000
Good catch Sunggook. […]
I put these into a std::array kSupportedSampleRates, I also did the same for bitrate.
timestamp_tracker_ =
std::make_unique<AudioTimestampHelper>(options.sample_rate);minor: can it also move to before setting initialized_ = true?
Done
if (FAILED(hr)) {
std::move(done_cb).Run(EncoderStatus::Codes::kEncoderInitializationError);
return;
}
Good idea to pull out the repeating code. […]
I do not think we can do this because it would require moving the callback into the function.
Patch Set #5, Line 320:
output_media_typeany reason not using SUCCEEDED(hr)?
We should be checking the hr here, fixed.
if (!initialized_) {
std::move(done_cb).Run(
EncoderStatus::Codes::kEncoderInitializeNeverCompleted);
return;
}
this should be before above DCHECK because these variables (channel_count_, etc) must be false if in […]
Done
Patch Set #5, Line 410:
not_accepting = true;
How about output processing doesn't handle data, then doesn't ProcessInput return MF_E_NOTACCEPTING […]
I don't think I understand your question. Are you saying that if we don't process the output data, then ProcessInput will always return MF_E_NOTACCEPTING? If so, that's correct, but we do process the output data below, as part of this loop.
Hopefully the new design is clearer, let me know if you still have questions.
Patch Set #5, Line 448:
= nullptrno need, same for every ComPtr init in this file.
Done
no need to set for out parameters when its return value have success/failure like HRESULT.
Done
if (FAILED(hr))
return hr;
it looks like we can use LOG_HR_ON_FAILURE like mf_audio_decoder. […]
Done
Patch Set #5, Line 480:
return E_FAIL;
E_FAIL is very useful, but at the same time it is very annoying from the caller perspective; can't f […]
We can actually remove this, since status will only be set if ProcessOutput returns MF_E_TRANSFORM_STREAM_CHANGE. Instead let's DCHECK this after the if (FAILED(hr)) above, since we don't expect any stream changes or have any way to handle them.
Patch Set #5, Line 528:
if (!initialized_) {same here, we need to check initialized_ before accessing member variables (DCHECK above).
Done
Patch Set #5, Line 539:
MF_E_TRANSFORM_NEED_MORE_INPUT
"It will stop calling ProcessOutput() once it returns MF_E_TRANSFORM_NEED_MORE_INPUT. […]
Right, this is also what we do here. If we get MF_E_NEED_MORE_INPUT, we won't run the error callback, and we also stop looping. Hopefully this is clearer in the new design.
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Austin Orion (Gerrit)
Attention is currently required from: Dale Curtis, Eugene Zemtsov, Steve Becker, Austin Orion, Sunggook Chue, Rahul Singh.
1 comment:
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #7, Line 732:
kSamplesPerFrame * 4Need to find out how much data the `mf_encoder_` keeps buffered, so that we know when we should keep checking for output. Experimentally it seems it keeps 2048 samples buffered, but need to confirm this.
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Austin Orion (Gerrit)
Attention is currently required from: Dale Curtis, Eugene Zemtsov, Steve Becker, Austin Orion, Sunggook Chue, Rahul Singh.
Austin Orion uploaded patch set #8 to this change.
Add MF AAC Encoder.
This CL introduces MFAudioEncoder which uses the Windows Media
Foundation's AAC encoder.
https://docs.microsoft.com/en-us/windows/win32/medfound/aac-encoder
The MF AAC encoder was designed to encode files, where all the data
is available at the start, and so the API is not perfectly suited
for stream encoding. Specifically, there is no callback or event
to let us know when output data is ready, or when it is ready for more
input.
We make this work with a state machine that is described by this
diagram:
https://mermaid.live/view/#pako:eNqFVNtu2kAQ_ZXpPkRGCn7gkbZIaXGkqJWSBt66lbXYA1jYa3cvrZDNv3cv1Dcc5Ynd2Tkz55wZXJOkTJEsyUGw6gjbNeUA3_GAPP1JiT8sP-3Eal0qhSnkGUcJTCC8lFJtmTxJSn5Z0FeW5yhgPl_BY67lcRSLuG300UbdM8zD-aqRiimEz3BysYwfGtiK84soE5TyWatKK4vwYAcx598aNT7x9q27206Nu8ZhZQo2sLEN6pqSbA_XZqbbU5ojJZeLxbsMB9yzXGIDr6i04EFASQe4MjIEwZoBwV90JrBcIEvPIDTn5nFGyWzWK2olKqGxr6olPgo5Cq0fXcMGHpIEK2WOdf3hKg6LSp3h7s6zQWePiMM201X0-tpYX2PES304ftH7PQpM61qyospRxhmPr8VgBUXGp0p4QWM1N1I28fO3HvfJnEiIUhg29icIWvHu7o0cEp12tL8nw-xOsJ14r4O9-gbjMm5Z_fHhD8tytsvN_rQOj168P6Ngv-8PO6_ITquu-7N7Gziwt5N2Q3Jg3mLavVuQnYpfmtIF42QXh6-aN2MuFt6R7zEberx4b3f6NcLOl4n_w6jweHqLyfHdgN5eEHJPChQFy1LzuastmBJ1xMJ8CZbmmDJxooTyi8nTVWo6RWmmSkGWttw9YVqVmzNP_t99zjpj5stZ-ODlH1c0454
Basically the process is as follows:
1. If we have input in our queue, give it to the encoder until we run
out or the encoder rejects it (because it is full)
2. Check the encoder for output until it stops producing
3. If, by our count, the encoder could produce more data, check again
4. If we have more input in the queue, by now, go back to 1
Each transition (e.g. from step 1 to step 2) is accomplished by
posting a task to a SequencedTaskRunner. This ensures that we can
safely access our mutable members (like the queue) without issue,
and we yield the thread frequently so we don't block for extended
periods of time.
This class will be used by MojoAudioEncoder to enable encoding to be
done in the GPU process when using WebCodecs and other consumers. See
these other related CLs:
3243816: webcodecs: Mojo interface, service and client for AudioEncoder | https://chromium-review.googlesource.com/c/chromium/src/+/3243816
3308655: webcodecs: Binding for Mojo Audio Encoder | https://chromium-review.googlesource.com/c/chromium/src/+/3308655
Bug: 1072056
Change-Id: I89b6a21407e5d7ff24a09569b34ffbdd8073ad67
---
M content/gpu/gpu_main.cc
M media/gpu/BUILD.gn
A media/gpu/windows/mf_audio_encoder.cc
A media/gpu/windows/mf_audio_encoder.h
4 files changed, 1,027 insertions(+), 0 deletions(-)
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Eugene Zemtsov, Steve Becker, Austin Orion, Sunggook Chue, Rahul Singh.
22 comments:
Patchset:
Seems you could add a simple unit test for this code that uses the encoder directly (it doesn't take a lot of deps). Can you do that?
File media/gpu/windows/mf_audio_encoder.h:
Patch Set #8, Line 32:
// Encodes PCM audio samples into AAC samples with ADTS headers on Win8 and rawI'm surprised to see this talk about Win7/8 vs 10/11 :) Can you add some clarity for what happens on more modern OS here?
Patch Set #8, Line 64:
// `Initialize` is synchronous and will call `done_cb` before returning.Generally our style is |done_cb| and just Initialize() in comments. There's no formal rule for this though, so up to you. (Change rest of file if you do change)
Patch Set #8, Line 117:
::base::TimeTicks capture_time,No leading ::, here and everywhere else.
Patch Set #8, Line 158:
std::deque<InputData> input_queue_;base::circular_deque
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #7, Line 732:
kSamplesPerFrame * 4
Need to find out how much data the `mf_encoder_` keeps buffered, so that we know when we should keep […]
Is there no callback based interface to let us know when data is ready?
File media/gpu/windows/mf_audio_encoder.cc:
I think we have this same function in dxva decoder. Maybe should go in a common MF utility class?
Patch Set #8, Line 113:
using GetClassObject =GetClassObjectFn ?
Patch Set #8, Line 117:
GetClassObject get_class_object = reinterpret_cast<GetClassObject>(good use for auto
Patch Set #8, Line 235:
DCHECK_GE(*min_input_buffer_size,Since this is using a result from a system call, I think you want a CHECK() instead of DCHECK to catch any bad drivers. If it actually starts crashing we can change it to a conditional.
Patch Set #8, Line 282:
// The caller supplies a `source_buffer` so we don't have to allocate a newWhy do we need this? Can't you just call ToInterleaved() directly into the dest_buffer?
Patch Set #8, Line 290:
audio_bus->ToInterleaved<media::SignedInt16SampleTypeTraits>(Does MFAudioFormat_Float not work?
Patch Set #8, Line 304:
memcpy(source_buffer.data(), dest_buffer_ptr, source_data_size);This copies dest_buffer_ptr data into source_buffer. I think that's backwards?
Patch Set #8, Line 382:
DCHECK_EQ(status, 0u);Since this is a system api call, you should probably handle this or CHECK() since we see lots of bad drivers and interceptors in the field.
Patch Set #8, Line 384:
if (output_data_container.pEvents != nullptr) {Drop != nullptr
Patch Set #8, Line 439:
: samples_in_encoder_(0),initialize these inline in the header file where possible. Less error prone.
Patch Set #8, Line 453:
DCHECK(!done_cb.is_null());Just DCHECK(done_cb) here and everywhere else drop is_null
Patch Set #8, Line 572:
DCHECK_EQ(audio_bus->channels(), channel_count_);Probably worth a hard error. I'm not confident in our encoding paths here yet.
Patch Set #8, Line 577:
::base::BindOnce(&MFAudioEncoder::EnqueueInput, base::Unretained(this),Unretained doesn't seem safe in this class. Can you explain why it is? I think you need a WeakPtrFactory.
Patch Set #8, Line 666:
break;If this happens, what triggers work later?
Patch Set #8, Line 691:
state_ == EncoderState::kFlushing);Helpful to add a << state_ here for debugging.
Patch Set #8, Line 782:
DCHECK(state_ == EncoderState::kFlushing);DCHECK_EQ
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Austin Orion (Gerrit)
Attention is currently required from: Dale Curtis, Eugene Zemtsov, Steve Becker, Sunggook Chue, Rahul Singh.
21 comments:
Patchset:
Seems you could add a simple unit test for this code that uses the encoder directly (it doesn't take […]
Definitely, I've hacked together a unit test for inner loop testing, and I'm working on polishing it up and posting. Thanks for adding a tracking comment so we don't miss this.
Patchset:
Thanks for taking such a quick look, Dale, I'll get to the rest of your comments tomorrow.
File media/gpu/windows/mf_audio_encoder.h:
Patch Set #8, Line 32:
// Encodes PCM audio samples into AAC samples with ADTS headers on Win8 and raw
I'm surprised to see this talk about Win7/8 vs 10/11 :) Can you add some clarity for what happens on […]
Whoops, this should say Win8+. Fixed.
Patch Set #8, Line 64:
// `Initialize` is synchronous and will call `done_cb` before returning.
Generally our style is |done_cb| and just Initialize() in comments. […]
I thought backticks were the new thing:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++-dos-and-donts.md#comment-styleLet me know if it would be better to match the style of the code in the surrounding files, otherwise I'll leave this. I will add trailing () to function names though.
Patch Set #8, Line 117:
::base::TimeTicks capture_time,No leading ::, here and everywhere else.
Done
Patch Set #8, Line 158:
std::deque<InputData> input_queue_;base::circular_deque
It seems we can't use base::circular_deque because it requires a copy constructor which we can't provide for this struct since it contains a base::OnceCallback. Let me know if I'm missing something.
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #7, Line 732:
kSamplesPerFrame * 4
Is there no callback based interface to let us know when data is ready?
Unfortunately no. The MF AAC encoder is a synchronous MF Transform so it has no notification mechanism. We must call GetOutputStatus to find out, but even then it could have enough data to eventually have output, it's just not ready yet. This helps us ensure that we don't leave samples in the encoder.
File media/gpu/windows/mf_audio_encoder.cc:
I think we have this same function in dxva decoder. […]
Good idea, I added this to the media/base/win/mf_helpers.cc. I'll look into either moving some more of these functions into there, or making use of the ones already there (e.g. for creating MFSamples).
Patch Set #8, Line 113:
using GetClassObject =GetClassObjectFn ?
done, and added _func to get_class_object_func
Patch Set #8, Line 117:
GetClassObject get_class_object = reinterpret_cast<GetClassObject>(good use for auto
Done
Patch Set #8, Line 235:
DCHECK_GE(*min_input_buffer_size,
Since this is using a result from a system call, I think you want a CHECK() instead of DCHECK to cat […]
Done
Patch Set #8, Line 282:
// The caller supplies a `source_buffer` so we don't have to allocate a newWhy do we need this? Can't you just call ToInterleaved() directly into the dest_buffer?
Would you look at that, we can! This is fixed.
Patch Set #8, Line 290:
audio_bus->ToInterleaved<media::SignedInt16SampleTypeTraits>(Does MFAudioFormat_Float not work?
I don't think so, the subtype is required to be MFAudioFormat_PCM, and the bits per sample must also be 16, which I think restricts us to non-float types.
https://docs.microsoft.com/en-us/windows/win32/medfound/aac-encoder#input-types
Patch Set #8, Line 304:
memcpy(source_buffer.data(), dest_buffer_ptr, source_data_size);
This copies dest_buffer_ptr data into source_buffer. […]
This has been removed.
Patch Set #8, Line 382:
DCHECK_EQ(status, 0u);
Since this is a system api call, you should probably handle this or CHECK() since we see lots of bad […]
Done
Patch Set #8, Line 384:
if (output_data_container.pEvents != nullptr) {Drop != nullptr
Done
Patch Set #8, Line 439:
: samples_in_encoder_(0),initialize these inline in the header file where possible. Less error prone.
Done
Patch Set #8, Line 453:
DCHECK(!done_cb.is_null());Just DCHECK(done_cb) here and everywhere else drop is_null
Done
Patch Set #8, Line 572:
DCHECK_EQ(audio_bus->channels(), channel_count_);Probably worth a hard error. I'm not confident in our encoding paths here yet.
Updated this to CHECK_EQ.
Patch Set #8, Line 666:
break;If this happens, what triggers work later?
If the encoder is not accepting, it's because its internal input buffers are full, which means that `samples_in_encoder_` will be large enough that we will call TryProcessOutput. From there we will see that `input_queue_` has data remaining in it, and we'll end up back here.
Patch Set #8, Line 782:
DCHECK(state_ == EncoderState::kFlushing);DCHECK_EQ
Done
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Eugene Zemtsov, Steve Becker, Austin Orion, Sunggook Chue, Rahul Singh.
3 comments:
File media/gpu/windows/mf_audio_encoder.h:
Patch Set #8, Line 64:
// `Initialize` is synchronous and will call `done_cb` before returning.I thought backticks were the new thing: […]
Huh, TIL, thanks for the reference!
Patch Set #8, Line 158:
std::deque<InputData> input_queue_;
It seems we can't use base::circular_deque because it requires a copy constructor which we can't pro […]
If you give the struct a move constructor I think it'll do the right thing?
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #7, Line 732:
kSamplesPerFrame * 4
Unfortunately no. […]
Ah, that's unfortunate. The MFT interface is a bit painful to use 😞
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Austin Orion (Gerrit)
Attention is currently required from: Dale Curtis, Eugene Zemtsov, Steve Becker, Sunggook Chue, Rahul Singh.
4 comments:
File media/gpu/windows/mf_audio_encoder.h:
Patch Set #8, Line 158:
std::deque<InputData> input_queue_;If you give the struct a move constructor I think it'll do the right thing?
Oh right. This is done.
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #7, Line 732:
kSamplesPerFrame * 4
Ah, that's unfortunate. […]
Asynchronous MFTs are better for sure, but yeah this one is pretty old and not very well suited for stream encoding.
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #8, Line 577:
::base::BindOnce(&MFAudioEncoder::EnqueueInput, base::Unretained(this),Unretained doesn't seem safe in this class. […]
Oh I thought the object just had to outlive the task runner, yeah these should be WeakPtrs. Fixed.
Patch Set #8, Line 691:
state_ == EncoderState::kFlushing);Helpful to add a << state_ here for debugging.
Is there a better way to do this for enum classes? I had to cast it to an int for logging, I had some trouble implementing the << operator (I guess because the enum is a private member of MFAudioEncoder).
Let me know, or if casting is the way to go then this is done.
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Eugene Zemtsov, Steve Becker, Austin Orion, Sunggook Chue, Rahul Singh.
Patch set 10:Code-Review +1
11 comments:
File media/base/win/mf_helpers.cc:
Patch Set #10, Line 29:
HRESULT CreateCOMObjectFromDll(const HMODULE dll,FWIW, I thought this was the old way of activating MFTs. Do we not want to use MFTEnumEx?
File media/gpu/windows/mf_audio_encoder.cc:
Good idea, I added this to the media/base/win/mf_helpers.cc. […]
Ack
Patch Set #8, Line 691:
state_ == EncoderState::kFlushing);
Is there a better way to do this for enum classes? I had to cast it to an int for logging, I had som […]
If you give the enum class a value type I think it might auto convert. I.e.,
enum class BlahBlah : uint8_t { ... }
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #10, Line 241:
HRESULT CreateMFSampleFromAudioBus(const std::unique_ptr<AudioBus>& audio_bus,Usually we pass const T& instead of unique_ptr<T>& if the function will never get a null value. Possibly you want to do the same above with ComPtr<T>&, but defer to you.
Patch Set #10, Line 335:
output_data_container.pSample = output_sample.Get();Do you know why we can't just reuse the same output_sample over and over? I'm pretty sure we can't since I hit issues myself doing that here:
https://chromium-review.googlesource.com/c/chromium/src/+/3251697/1/media/filters/win/media_foundation_audio_decoder.cc#195... but I'm not sure why it wouldn't work since we're copying out of the sample every time.
Patch Set #10, Line 375:
base::TimeDelta duration = base::Nanoseconds(sample_duration * 100);https://source.chromium.org/chromium/chromium/src/+/main:base/time/time.h;l=653;drc=257f70ebc81ee08b2c0e9d1ad3c984979ccb351a might be what you want.
Patch Set #10, Line 438:
std::move(done_cb).Run(EncoderStatus::Codes::kEncoderInitializationError);Should this be not supported error?
Patch Set #10, Line 572:
<< static_cast<int>(state_);Probably worth a "state_ == " here and in the other places.
Patch Set #10, Line 591:
LONGLONG duration = timestamp_tracker_->GetFrameDuration(audio_bus->frames())Ditto on TS helpers.
Patch Set #10, Line 610:
task_runner_->PostTask(FROM_HERE,Why not try to process input immediately?
Patch Set #10, Line 703:
task_runner_->PostTask(FROM_HERE,Should this be a post delayed task?
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Eugene Zemtsov, Steve Becker, Austin Orion, Sunggook Chue, Rahul Singh.
1 comment:
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #10, Line 375:
base::TimeDelta duration = base::Nanoseconds(sample_duration * 100);https://source.chromium.org/chromium/chromium/src/+/main:base/time/time. […]
(Note they might not be worth the pain, but perhaps we should contribute some useful ones to base/time.h since this conversion comes up everywhere in the media/audio stacks).
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Eugene Zemtsov (Gerrit)
Attention is currently required from: Zhenyao Mo, Steve Becker, Austin Orion, Sunggook Chue, Rahul Singh.
Eugene Zemtsov would like Zhenyao Mo to review this change authored by Austin Orion.
M media/base/win/mf_helpers.cc
M media/base/win/mf_helpers.h
M media/gpu/BUILD.gn
M media/gpu/windows/dxva_video_decode_accelerator_win.cc
A media/gpu/windows/mf_audio_encoder.cc
A media/gpu/windows/mf_audio_encoder.h
7 files changed, 1,038 insertions(+), 27 deletions(-)
Eugene Zemtsov (Gerrit)
Attention is currently required from: Zhenyao Mo, Steve Becker, Austin Orion, Sunggook Chue, Rahul Singh.
1 comment:
Patchset:
Adding zmo@ for content/gpu
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Austin Orion (Gerrit)
Attention is currently required from: Dale Curtis, Zhenyao Mo, Steve Becker, Sunggook Chue, Rahul Singh.
9 comments:
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #8, Line 691:
state_ == EncoderState::kFlushing);If you give the enum class a value type I think it might auto convert. I.e., […]
I forgot to mention that I tried that as well. The only thing that worked was making it an enum instead of an enum class, but I'd rather cast it than do that. I'll just leave it how it is, I was just wondering if there was something I was missing.
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #10, Line 241:
HRESULT CreateMFSampleFromAudioBus(const std::unique_ptr<AudioBus>& audio_bus,Usually we pass const T& instead of unique_ptr<T>& if the function will never get a null value. […]
Done for AudioBus. I left the ComPtr<T> because the functions we're calling on them aren't marked const.
Patch Set #10, Line 335:
output_data_container.pSample = output_sample.Get();
Do you know why we can't just reuse the same output_sample over and over? I'm pretty sure we can't s […]
Seems to be working for me, maybe it is an issue specifically with the decoder? I see our implementation downstream also does not reuse output buffers. What was the error specifically? Maybe I am overlooking something.
Patch Set #10, Line 375:
base::TimeDelta duration = base::Nanoseconds(sample_duration * 100);
(Note they might not be worth the pain, but perhaps we should contribute some useful ones to base/ti […]
Good suggestion! I think MFTIME would be a better candidate though. It works great for us because it's a typedef for LONGLONG rather than a struct like FILETIME. And it avoids confusion like what's described in this bug, since MFTIME is just a unit and not an absolute time:
https://bugs.chromium.org/p/chromium/issues/detail?id=989694I don't think I understand TimeTicks well enough to add a conversion for that class and maintain its guarantees, so I've just added TimeDelta helpers for this.
Patch Set #10, Line 438:
std::move(done_cb).Run(EncoderStatus::Codes::kEncoderInitializationError);Should this be not supported error?
I could see it either way, but I'll go ahead and update this to kEncoderUnsupportedCodec.
Patch Set #10, Line 572:
<< static_cast<int>(state_);Probably worth a "state_ == " here and in the other places.
Sorry, I don't understand, what are you suggestion we compare state_ to?
Patch Set #10, Line 591:
LONGLONG duration = timestamp_tracker_->GetFrameDuration(audio_bus->frames())Ditto on TS helpers.
Done
Patch Set #10, Line 610:
task_runner_->PostTask(FROM_HERE,Why not try to process input immediately?
I was trying to make the public functions callable from any sequence, and then post tasks to our internal functions onto our SequencedTaskRunner, but that had some issues.
Now I've updated this class so it must be called on a SequencedTaskRunner (which works fine since we expect to be wrapped in an OffloadingAudioEncoder) and so now we can call these functions immediately instead of posting tasks.
Patch Set #10, Line 703:
task_runner_->PostTask(FROM_HERE,Should this be a post delayed task?
I thought about it, but I couldn't decided on a reasonable delay amount. This also breaks the test that I have because RunUntilIdle exits before running delayed tasks. If it's not a deal breaker for you, I'll leave this.
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Austin Orion (Gerrit)
Attention is currently required from: Dale Curtis, Zhenyao Mo, Peter Kasting, Steve Becker, Sunggook Chue, Rahul Singh.
Austin Orion would like Peter Kasting to review this change.
M base/time/time.h
M base/time/time_win.cc
M base/win/windows_types.h
M content/gpu/gpu_main.cc
M media/base/win/mf_helpers.cc
M media/base/win/mf_helpers.h
M media/gpu/BUILD.gn
M media/gpu/windows/dxva_video_decode_accelerator_win.cc
A media/gpu/windows/mf_audio_encoder.cc
A media/gpu/windows/mf_audio_encoder.h
M media/renderers/win/media_foundation_stream_wrapper.cc
11 files changed, 1,140 insertions(+), 34 deletions(-)
Austin Orion (Gerrit)
Attention is currently required from: Dale Curtis, Zhenyao Mo, Peter Kasting, Steve Becker, Sunggook Chue, Rahul Singh.
1 comment:
Patchset:
Hi pkasting@, PTAL at some MFTIME helpers that I added to time.h
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Austin Orion (Gerrit)
Attention is currently required from: Dale Curtis, Zhenyao Mo, Peter Kasting, Steve Becker, David Bienvenu, Sunggook Chue, Rahul Singh.
Austin Orion would like David Bienvenu to review this change.
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Austin Orion (Gerrit)
Attention is currently required from: Dale Curtis, Zhenyao Mo, Peter Kasting, Steve Becker, David Bienvenu, Sunggook Chue, Rahul Singh.
1 comment:
Patchset:
Hi davidbienvenu@, PTAL at the addition of MFTIME to base/win/windows_types.h
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Zhenyao Mo (Gerrit)
Attention is currently required from: Dale Curtis, Peter Kasting, Steve Becker, David Bienvenu, Austin Orion, Sunggook Chue, Rahul Singh.
Patch set 12:Code-Review +1
1 comment:
Patchset:
content/gpu lgtm
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Peter Kasting, Steve Becker, David Bienvenu, Austin Orion, Sunggook Chue, Rahul Singh.
Patch set 12:Code-Review +1
9 comments:
File base/time/time.h:
Patch Set #12, Line 122:
static TimeDelta FromMfTime(MFTIME mf_time);I'd probably land these in another CL instead for cleanliness. base/ team is sure to want some tests added for this.
File media/base/win/mf_helpers.cc:
Patch Set #10, Line 29:
HRESULT CreateCOMObjectFromDll(const HMODULE dll,
FWIW, I thought this was the old way of activating MFTs. […]
Bump on this Q.
File media/gpu/windows/mf_audio_encoder.h:
Patch Set #12, Line 49:
// `OffloadingAudioEncoder` is a helpful wrapper for this.Mistake?
Patch Set #12, Line 57:
MFAudioEncoder();Do we need both constructors? The one with which takes a task runner should be sufficient.
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #10, Line 241:
HRESULT CreateMFSampleFromAudioBus(const std::unique_ptr<AudioBus>& audio_bus,
Done for AudioBus. […]
Usually that means you want T* instead.
Patch Set #10, Line 335:
output_data_container.pSample = output_sample.Get();
Seems to be working for me, maybe it is an issue specifically with the decoder? I see our implementa […]
Hmm, I don't recall, it's certainly possible it was user error.
Patch Set #10, Line 572:
<< static_cast<int>(state_);
Sorry, I don't understand, what are you suggestion we compare state_ to?
I just meant add the string "state_ == " so that the text isn't
"Check failed! 5" but "Check failed! state_ == 5"
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #12, Line 615:
if (audio_bus->frames() <= 0) {0 sized AudioBus is impossible.
Patch Set #12, Line 727:
!flush_cb)Needs {}
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
David Bienvenu (Gerrit)
Attention is currently required from: Peter Kasting, Steve Becker, Austin Orion, Sunggook Chue, Rahul Singh.
2 comments:
File media/gpu/windows/mf_audio_encoder.h:
Doing so will cause an error, if I'm reading the code correctly, so you might want to incorporate that into your comment.
File media/gpu/windows/mf_audio_encoder.cc:
like above, add << "state_ == " << static_cast<int>(state_);
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Austin Orion (Gerrit)
Attention is currently required from: Steve Becker, Sunggook Chue, Rahul Singh.
Austin Orion has uploaded this change for review.
M content/gpu/gpu_main.cc
M media/base/win/mf_helpers.cc
M media/base/win/mf_helpers.h
M media/gpu/BUILD.gn
M media/gpu/windows/dxva_video_decode_accelerator_win.cc
A media/gpu/windows/mf_audio_encoder.cc
A media/gpu/windows/mf_audio_encoder.h
7 files changed, 1,131 insertions(+), 27 deletions(-)
Austin Orion (Gerrit)
Attention is currently required from: Steve Becker, Sunggook Chue, Rahul Singh.
2 comments:
Patchset:
Undid the changes to time.h and media_types.h so moving pkasting@ and davidbienvenu@ to CC.
File base/time/time.h:
Patch Set #12, Line 122:
static TimeDelta FromMfTime(MFTIME mf_time);
I'd probably land these in another CL instead for cleanliness. […]
Done.
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Austin Orion (Gerrit)
Attention is currently required from: Steve Becker, Sunggook Chue, Rahul Singh.
7 comments:
File media/base/win/mf_helpers.cc:
Patch Set #10, Line 29:
HRESULT CreateCOMObjectFromDll(const HMODULE dll,
Bump on this Q.
Should have ack'd, I was looking into this. You're right, I've updated to using MftEnumEx.
I undid the changes to mf_helpers and dxga_video_decode_accelerator_win since there's no duplication anymore.
File media/gpu/windows/mf_audio_encoder.h:
Patch Set #12, Line 49:
// `OffloadingAudioEncoder` is a helpful wrapper for this.Mistake?
I'm referencing this class
https://source.chromium.org/chromium/chromium/src/+/main:media/base/offloading_audio_encoder.h?q=offloadingaudioencoderSimilar to how it's used here:
https://source.chromium.org/chromium/chromium/src/+/main:media/mojo/services/gpu_mojo_media_client.cc;l=127?q=offloadingaudioencoderOtherwise, the caller will have to PostTask all of our functions, unless they're ok with us running on the current sequence. I just wanted to make it clear that even though you passed us a task_runner, you also need to call us on the same task runner, and OffloadingAudioEncoder makes that easy.
Patch Set #12, Line 57:
MFAudioEncoder();Do we need both constructors? The one with which takes a task runner should be sufficient.
Just for convenience, I'll remove it. It makes it more explicit that it needs to be run on a sequence.
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #10, Line 335:
output_data_container.pSample = output_sample.Get();
Hmm, I don't recall, it's certainly possible it was user error.
I will run my tests with a sanitizer and check tomorrow.
Patch Set #10, Line 572:
<< static_cast<int>(state_);
I just meant add the string "state_ == " so that the text isn't […]
Good idea! Done.
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #12, Line 615:
if (audio_bus->frames() <= 0) {
0 sized AudioBus is impossible.
Oh I see the DCHECK in there now, I'll remove this.
Patch Set #12, Line 727:
!flush_cb)Needs {}
Done
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Eugene Zemtsov (Gerrit)
Attention is currently required from: Steve Becker, Austin Orion, Sunggook Chue, Rahul Singh.
2 comments:
File media/gpu/windows/mf_audio_encoder.h:
Does it have to be MTA? I'd like to run it on a runner created by ThreadPool::CreateCOMSTATaskRunner()
Patch Set #14, Line 175:
input_data_buffer_I don't think this variable is ever used.
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Austin Orion (Gerrit)
Attention is currently required from: Eugene Zemtsov, Steve Becker, Sunggook Chue, Rahul Singh.
4 comments:
File media/gpu/windows/mf_audio_encoder.h:
Does it have to be MTA? I'd like to run it on a runner created by ThreadPool::CreateCOMSTATaskRunner […]
No, it can be STA too. I've updated this comment and the ASSERT in Initialize.
Patch Set #14, Line 175:
input_data_buffer_I don't think this variable is ever used.
Whoops, removed.
File media/gpu/windows/mf_audio_encoder.h:
Doing so will cause an error, if I'm reading the code correctly, so you might want to incorporate th […]
I was looking at other encoders and it seems this is unique to us, so I think I'll instead add another queue to store input received while flushing. Then we can start work on that input once the flush completes.
I've removed this sentence from the comment since it no longer applies.
File media/gpu/windows/mf_audio_encoder.cc:
like above, add << "state_ == " << static_cast<int>(state_);
Done
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Eugene Zemtsov, Steve Becker, Austin Orion, Sunggook Chue, Rahul Singh.
Patch set 17:Code-Review +1
1 comment:
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #17, Line 72:
constexpr const wchar_t kAacDllName[] = L"mfaacenc.dll";Do we even need the DLL preloads anymore now that you're using MFTEnumEx?
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Austin Orion (Gerrit)
Attention is currently required from: Eugene Zemtsov, Steve Becker, Sunggook Chue, Rahul Singh.
2 comments:
Patchset:
Definitely, I've hacked together a unit test for inner loop testing, and I'm working on polishing it […]
This is done.
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #17, Line 72:
constexpr const wchar_t kAacDllName[] = L"mfaacenc.dll";Do we even need the DLL preloads anymore now that you're using MFTEnumEx?
Yes, the call to ActivateObject on line 129 will cause mfaacenc.dll to be loaded if it isn't already. And we need mfplat.dll in order to call MFTEnumEx.
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Eugene Zemtsov, Steve Becker, Austin Orion, Sunggook Chue, Rahul Singh.
Patch set 18:Code-Review +1
1 comment:
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #17, Line 72:
constexpr const wchar_t kAacDllName[] = L"mfaacenc.dll";
Yes, the call to ActivateObject on line 129 will cause mfaacenc. […]
Interesting, did you verify this? We were able to get away with no preloading the av1 MFT for some reason when loading this way, so I thought it might be similar.
MFPlat is already preloaded by the video decoders, so that one is fine, but the less we have to preload the better.
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Austin Orion (Gerrit)
Attention is currently required from: Dale Curtis, Eugene Zemtsov, Steve Becker, Sunggook Chue, Rahul Singh.
1 comment:
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #17, Line 72:
constexpr const wchar_t kAacDllName[] = L"mfaacenc.dll";
Interesting, did you verify this? We were able to get away with no preloading the av1 MFT for some r […]
I haven't run this in a sandbox to see if it fails, but when I run media_unittests under WinDbg and set a breakpoint with `sxe ld mfaacenc` I see the breakpoint hit on the call to ActivateObject. I've also debugged a few things and have stepped into mfaacenc, so we're definitely running code from there. IIUC, the sandbox would prevent us from loading the .dll after it's initialized, so I'm pretty sure we have to preload it.
I'm not sure why the av1 MFT works without preloading, maybe when MF is loaded it loads these codec .dlls by default? I can ask around and try to find out.
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Eugene Zemtsov, Steve Becker, Austin Orion, Sunggook Chue, Rahul Singh.
1 comment:
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #17, Line 72:
constexpr const wchar_t kAacDllName[] = L"mfaacenc.dll";
I haven't run this in a sandbox to see if it fails, but when I run media_unittests under WinDbg and […]
Okay, it might be worth leaving out the sandbox initialization stuff until we can confirm that then since it's not necessary for this patch (only the one which wires this up)
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Eugene Zemtsov (Gerrit)
Attention is currently required from: Steve Becker, Austin Orion, Sunggook Chue, Rahul Singh.
1 comment:
Patchset:
I ran media_unittests on my workstation. One of them failed:
AAC/AudioEncodersTest.Timestamps/2, where GetParam() = 12-byte object <01-00 00-00 01-00 00-00 80-BB 00-00>
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Eugene Zemtsov (Gerrit)
Attention is currently required from: Steve Becker, Austin Orion, Sunggook Chue, Rahul Singh.
1 comment:
File media/gpu/windows/mf_audio_encoder.h:
Patch Set #18, Line 185:
task_runner_I've noticed that |task_runner_| is only used for calling TryProcessInput() and TryProcessOutput() and these functions assert that the sequence |sequence_checker_| as all regular public methods.
Do we even need this runner then? Can't we just synchronously call TryProcessInput() and TryProcessOutput()?
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Austin Orion (Gerrit)
Attention is currently required from: Eugene Zemtsov, Steve Becker, Sunggook Chue, Rahul Singh.
3 comments:
Patchset:
I ran media_unittests on my workstation. One of them failed: […]
Yes, I am still investigating this failure. I'm not sure why it's failing when both are using an AudioTimestampHelper, but I'll continue debugging it.
File media/gpu/windows/mf_audio_encoder.h:
Patch Set #18, Line 185:
task_runner_
I've noticed that |task_runner_| is only used for calling TryProcessInput() and TryProcessOutput() a […]
We PostTasks to yield the thread, otherwise we would block while waiting for the encoder to produce output. The encoder won't accept input while its buffer is full, and calls to GetOutputStatus will return false if the input data isn't finished encoding. During this period, we will spin if the calls were synchronous. If the input queue were to fill up faster than we could process it, we could block for a very long time.
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #17, Line 72:
constexpr const wchar_t kAacDllName[] = L"mfaacenc.dll";
Okay, it might be worth leaving out the sandbox initialization stuff until we can confirm that then […]
Sure, I've removed the call from gpu_main.cc
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Eugene Zemtsov (Gerrit)
Attention is currently required from: Austin Orion.
3 comments:
Patchset:
I have a webcodecs integration CL pending, but unfortunately when I try to do full cycle encode/decode test in using webcodecs API I get this error from ffmpeg decoder:
ERROR:ffmpeg_decoding_loop.cc(31)] Failed to send packet for decoding: -1094995529 msg: Invalid data found when processing input
I don't think it's showstopper though, we can figure out what's the matter after this CL is submitted.
File content/gpu/gpu_main.cc:
Patch Set #19, Line 86:
#include "media/gpu/windows/mf_audio_encoder.h"Not needed
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #17, Line 72:
constexpr const wchar_t kAacDllName[] = L"mfaacenc.dll";
Sure, I've removed the call from gpu_main. […]
Yeah, I have a pending integration CL. And the encoder seems to work fine without this call.
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Austin Orion (Gerrit)
Attention is currently required from: Eugene Zemtsov.
4 comments:
Patchset:
Yes, I am still investigating this failure. […]
This test is fixed. The issue was that we were using the timestamp provided by the encoder, which (when the inputs are large) undercounts due to summing the (truncated) durations. The solution was to add an output_timestamp_tracker_ and use that instead.
File content/gpu/gpu_main.cc:
Patch Set #19, Line 86:
#include "media/gpu/windows/mf_audio_encoder.h"Not needed
Done
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #10, Line 335:
output_data_container.pSample = output_sample.Get();
I will run my tests with a sanitizer and check tomorrow.
Passes fine with under ASAN. Having troubles running UBSAN and MSAN though, but I think we're fine here.
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #17, Line 72:
constexpr const wchar_t kAacDllName[] = L"mfaacenc.dll";
Yeah, I have a pending integration CL. And the encoder seems to work fine without this call.
I asked around and the answer is complicated, but it's not unexpected that loading this .dll would succeed since it is relatively small and self contained. Happy to share more details if you'd like, but you're correct that we don't need to explicitly load mfaacenc.dll. So, I've removed it.
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Austin Orion (Gerrit)
Attention is currently required from: Eugene Zemtsov.
1 comment:
Patchset:
I have a webcodecs integration CL pending, but unfortunately when I try to do full cycle encode/deco […]
I think this was because I was reusing the encoder's input options for the decoder, which it didn't like. I've reverted the test back to using constants specifically for the decoder, which fixed the test failures.
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Eugene Zemtsov (Gerrit)
Attention is currently required from: Austin Orion.
Patch set 25:Code-Review +1Commit-Queue +2
Austin Orion (Gerrit)
Attention is currently required from: Rahul Singh.
1 comment:
File media/gpu/windows/mf_audio_encoder.cc:
Patch Set #3, Line 234:
kEncoderUnsupportedConfigI think we can add detail by providing messages, but it would be useful to have a more detailed enum […]
I'm going to address this in a follow up CL.
To view, visit change 3293969. To unsubscribe, or for help writing mail filters, visit settings.
Austin Orion (Gerrit)
Attention is currently required from: Eugene Zemtsov, Austin Orion, Rahul Singh.
Patch set 25:Commit-Queue +2
Eugene Zemtsov (Gerrit)
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3293969
Reviewed-by: Zhenyao Mo <z...@chromium.org>
Reviewed-by: Dale Curtis <dalec...@chromium.org>
Reviewed-by: Eugene Zemtsov <eug...@chromium.org>
Commit-Queue: Eugene Zemtsov <eug...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#978595}
---
M media/audio/BUILD.gn
M media/audio/audio_encoders_unittest.cc
M media/audio/audio_opus_encoder.cc
M media/gpu/BUILD.gn
A media/gpu/windows/mf_audio_encoder.cc
A media/gpu/windows/mf_audio_encoder.h
6 files changed, 1,724 insertions(+), 151 deletions(-)