Closed
Bug 1432779
Opened 6 years ago
Closed 6 years ago
Have cubeb use same channel configuration as Windows/FFmpeg and rework cubeb_mixer
Categories
(Core :: Audio/Video: cubeb, enhancement, P2)
Core
Audio/Video: cubeb
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: jya, Assigned: jya)
References
(Depends on 2 open bugs)
Details
Attachments
(14 files, 1 obsolete file)
59 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
u480271
:
review+
padenot
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
u480271
:
review+
padenot
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
u480271
:
review+
padenot
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
cubeb uses its own channel layout convention (that comes from ITU BS.775). However, no system use anything similar except the mac audio unit. We should use the Windows WAVEFORMAT layout, which is what both FFmpeg, Pulse and Windows are using. Preliminary work was done in bug 1431221, which now use the same bitmask for layout definition and the same channel ordering.
Updated•6 years ago
|
Rank: 15
Priority: -- → P2
Assignee | ||
Updated•6 years ago
|
Summary: Have cubeb use same channel configuration as Windows/FFmpeg → Have cubeb use same channel configuration as Windows/FFmpeg and rework cubeb_mixer
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 15•6 years ago
|
||
mozreview-review |
Comment on attachment 8957699 [details] Bug 1432779 - P4. Remove the concept of preferred layout. https://reviewboard.mozilla.org/r/226628/#review232636 LGTM
Attachment #8957699 -
Flags: review?(dglastonbury) → review+
Comment 16•6 years ago
|
||
mozreview-review |
Comment on attachment 8957701 [details] Bug 1432779 - P6. Remove CHANNEL_MONO concept. https://reviewboard.mozilla.org/r/226632/#review232638
Attachment #8957701 -
Flags: review?(dglastonbury) → review+
Comment 17•6 years ago
|
||
mozreview-review |
Comment on attachment 8957704 [details] Bug 1432779 - P9. Rework cubeb_mixer. https://reviewboard.mozilla.org/r/226638/#review232640
Attachment #8957704 -
Flags: review?(dglastonbury) → review+
Comment 18•6 years ago
|
||
Opened PR against cubeb with patches: https://github.com/kinetiknz/cubeb/pull/421
Comment 19•6 years ago
|
||
mozreview-review |
Comment on attachment 8957696 [details] Bug 1432779 - P1. Fix ChannelLayout calculation for uncommon layouts. https://reviewboard.mozilla.org/r/226618/#review233116
Attachment #8957696 -
Flags: review?(padenot) → review+
Comment 20•6 years ago
|
||
mozreview-review |
Comment on attachment 8957697 [details] Bug 1432779 - P2. Add L3F2_BACK layout shortcuts. https://reviewboard.mozilla.org/r/226624/#review233118
Attachment #8957697 -
Flags: review?(padenot) → review+
Comment 21•6 years ago
|
||
mozreview-review |
Comment on attachment 8957698 [details] Bug 1432779 - P3. Properly set channel map layout after seeking. https://reviewboard.mozilla.org/r/226626/#review233120
Attachment #8957698 -
Flags: review?(padenot) → review+
Comment 22•6 years ago
|
||
mozreview-review |
Comment on attachment 8957699 [details] Bug 1432779 - P4. Remove the concept of preferred layout. https://reviewboard.mozilla.org/r/226628/#review233122
Attachment #8957699 -
Flags: review?(padenot) → review+
Comment 23•6 years ago
|
||
mozreview-review |
Comment on attachment 8957700 [details] Bug 1432779 - P5. Remove no longer used variables. https://reviewboard.mozilla.org/r/226630/#review233124
Attachment #8957700 -
Flags: review?(padenot) → review+
Comment 24•6 years ago
|
||
mozreview-review |
Comment on attachment 8957701 [details] Bug 1432779 - P6. Remove CHANNEL_MONO concept. https://reviewboard.mozilla.org/r/226632/#review233128
Attachment #8957701 -
Flags: review?(padenot) → review+
Comment 25•6 years ago
|
||
mozreview-review |
Comment on attachment 8957702 [details] Bug 1432779 - P7. Use typedef rather than actual type. https://reviewboard.mozilla.org/r/226634/#review233130
Attachment #8957702 -
Flags: review?(padenot) → review+
Comment 26•6 years ago
|
||
mozreview-review |
Comment on attachment 8957703 [details] Bug 1432779 - P8. Add cubeb_sample_size convenience method. https://reviewboard.mozilla.org/r/226636/#review233132 ::: media/libcubeb/src/cubeb_utils.cpp:2 (Diff revision 1) > +/* > + * Copyright © 2011 Mozilla Foundation Bump the year. ::: media/libcubeb/src/cubeb_utils.cpp:19 (Diff revision 1) > + case CUBEB_SAMPLE_S16BE: > + return sizeof(int16_t); > + case CUBEB_SAMPLE_FLOAT32LE: > + case CUBEB_SAMPLE_FLOAT32BE: > + return sizeof(float); > + } Add a default with an abort or something.
Attachment #8957703 -
Flags: review?(padenot) → review+
Comment 27•6 years ago
|
||
mozreview-review |
Comment on attachment 8957704 [details] Bug 1432779 - P9. Rework cubeb_mixer. https://reviewboard.mozilla.org/r/226638/#review233134 ::: media/libcubeb/src/cubeb_mixer.cpp:8 (Diff revision 1) > * > * This program is made available under an ISC-style license. See the > * accompanying file LICENSE for details. > + * > + * Adapted from code based on libswresample's rematrix.c > + * Copyright (C) 2011-2012 Michael Niedermayer (michaelni@gmx.at) I don't think we can say that this is ISC and say it's derived from LGPL, but we'll clear that up. ::: media/libcubeb/src/cubeb_mixer.cpp:71 (Diff revision 1) > }; > > -// The downmix matrix from TABLE 2 in the ITU-R BS.775-3[1] defines a way to > -// convert 3F2 input data to 1F, 2F, 3F, 2F1, 3F1, 2F2 output data. We extend it > -// to convert 3F2-LFE input data to 1F, 2F, 3F, 2F1, 3F1, 2F2 and their LFEs > -// output data. > +unsigned int cubeb_channel_layout_nb_channels(cubeb_channel_layout x) > +{ > +#if __GNUC__ || __clang__ > + return __builtin_popcount (x); For MSVC: __popcnt https://docs.microsoft.com/en-us/cpp/intrinsics/popcnt16-popcnt-popcnt64 ::: media/libcubeb/src/cubeb_mixer.cpp:105 (Diff revision 1) > + return true; > + } > return false; > } > > - unsigned int in_channels = CUBEB_CHANNEL_LAYOUT_MAPS[in_layout].channels; > + static cubeb_channel_layout clean_layout(cubeb_channel_layout layout) Please explain what this intends to achieve. ::: media/libcubeb/src/cubeb_mixer.cpp:149 (Diff revision 1) > - unsigned int in_channels = CUBEB_CHANNEL_LAYOUT_MAPS[in_layout].channels; > - unsigned int out_channels = CUBEB_CHANNEL_LAYOUT_MAPS[out_layout].channels; > + cubeb_sample_format _format; > + cubeb_channel_layout _in_ch_layout; ///< input channel layout > + cubeb_channel_layout _out_ch_layout; ///< output channel layout > + uint32_t _in_ch_count; ///< input channel count > + uint32_t _out_ch_count; ///< output channel count > + float _surround_mix_level = C_30DB; ///< surround mixing level const, and below. ::: media/libcubeb/src/cubeb_mixer.cpp:305 (Diff revision 1) > - for (unsigned int j = 0; j < out_channels; ++j) { > - assert(i + j < in_len && out_index + j < out_len); > + LOG("Output Layout %x is not supported", _out_ch_layout); > + return -1; > - out[out_index + j] = in[i + j]; > - } > + } > + > + for (uint32_t i = 0; i < FF_ARRAY_ELEMS(matrix); i++) { Here is the perfect place for a comment block that explains the general strategy used here, for the matrixing. ::: media/libcubeb/src/cubeb_mixer.cpp:314 (Diff revision 1) > -} > + } > > + unaccounted = in_ch_layout & ~out_ch_layout; > > -template<typename T> > -void > + if (unaccounted & CHANNEL_FRONT_CENTER) { > + if ((out_ch_layout & AV_CH_LAYOUT_STEREO) == AV_CH_LAYOUT_STEREO) { I think what you really want here is an assert. Real weird way of encoding this. And this pattern occurs quite often. Weird. ::: media/libcubeb/src/cubeb_mixer.cpp:347 (Diff revision 1) > + } else if (out_ch_layout & CHANNEL_SIDE_LEFT) { > + matrix[SIDE_LEFT][BACK_CENTER] += M_SQRT1_2; > + matrix[SIDE_RIGHT][BACK_CENTER] += M_SQRT1_2; > + } else if (out_ch_layout & CHANNEL_FRONT_LEFT) { > + if (_matrix_encoding == AV_MATRIX_ENCODING_DOLBY || > + _matrix_encoding == AV_MATRIX_ENCODING_DPLII) { As mentionned on irc, maybe we could drop those branches and also the `_matrix_encoding` member, since they add a fair bit of complexity in the setup. ::: media/libcubeb/src/cubeb_mixer.cpp:483 (Diff revision 1) > + } else { > + assert(false); > - } > + } > -} > + } > > -template<typename T> > + for (uint32_t out_i = 0, i = 0; i < 32; i++) { Can you simply add a comment block saying that is the normalization part of the matrix setup ? ::: media/libcubeb/src/cubeb_mixer.cpp:506 (Diff revision 1) > + } > + maxcoef = std::max(maxcoef, sum); > + out_i++; > + } > > - unsigned int in_channels = stream_params->channels; > + if (_rematrix_volume < 0) { This is effectively a constant in the code, and this cannot happen. ::: media/libcubeb/src/cubeb_mixer.cpp:511 (Diff revision 1) > - unsigned int in_channels = stream_params->channels; > - unsigned int out_channels = mixer_params->channels; > + if (_rematrix_volume < 0) { > + maxcoef = -_rematrix_volume; > + } > > - /* Either way, if we have 2 or more channels, the first two are L and R. */ > - /* If we are playing a mono stream over stereo speakers, copy the data over. */ > + if (_format == CUBEB_SAMPLE_S16NE) { > + maxval = 1.0; Aren't those branches backward ? ::: media/libcubeb/src/cubeb_mixer.cpp:524 (Diff revision 1) > + for (uint32_t j = 0; j < CHANNELS_MAX; j++) { > + _matrix[i][j] /= maxcoef; > - } > + } > } > > - /* Check if more channels. */ > + if (_rematrix_volume > 0 && _rematrix_volume != 1) { This is a noop, `_rematrix_volume` is always 1.0 for us. ::: media/libcubeb/src/cubeb_mixer.cpp:532 (Diff revision 1) > + _matrix[i][j] *= _rematrix_volume; > + } > } > > - /* Put silence in remaining channels. */ > - for (long i = 0, o = 0; i < inframes; ++i, o += out_channels) { > + if (_format == CUBEB_SAMPLE_FLOAT32NE) { > + for (uint32_t i = 0; i < FF_ARRAY_ELEMS(_matrix[0]); i++) { Shouldn't this be `FF_ARRAY_ELEMS(_matrix)`? ::: media/libcubeb/src/cubeb_mixer.cpp:584 (Diff revision 1) > + } > > -template<typename T> > -struct cubeb_mixer_impl : public cubeb_mixer { > - explicit cubeb_mixer_impl(unsigned int d) > - : direction(d) > + return 0; > +} > + > +static int rematrix(const MixerContext * s, void * out, void * in, uint32_t frames) I'm thinking it would be more elegant, probably more efficient and not too hard to do the dispatch on the type using templates, since your sum and copy functions are templated themselves. Would lead to code that is much cleaner as well.
Attachment #8957704 -
Flags: review?(padenot) → review-
Comment 28•6 years ago
|
||
mozreview-review |
Comment on attachment 8957705 [details] Bug 1432779 - P10. Fix remixing on mac, windows and pulse backend. https://reviewboard.mozilla.org/r/226640/#review233178 A few comments and questions, but looks good overall. ::: media/libcubeb/include/cubeb.h:220 (Diff revision 1) > + CUBEB_LAYOUT_3F4_LFE = CHANNEL_FRONT_LEFT | CHANNEL_FRONT_RIGHT | > + CHANNEL_FRONT_CENTER | CHANNEL_LOW_FREQUENCY | > + CHANNEL_BACK_LEFT | CHANNEL_BACK_RIGHT | > + CHANNEL_SIDE_LEFT | CHANNEL_SIDE_RIGHT, > + // FFmpeg layout compatible definition > + AV_CH_LAYOUT_STEREO = CHANNEL_FRONT_LEFT | CHANNEL_FRONT_RIGHT, Is the intent here to expose this? Do we have a reason to do so? ::: media/libcubeb/src/cubeb_audiounit.cpp:538 (Diff revision 1) > } > + > /* Get output buffer. */ > + if (stm->mixer) { > + // If remixing needs to occur, we can't directly work in our final > + // destination buffer as data may be overwritten or too small to start with. Double check that this affirmation is correct. We're configuring the size of the buffer the callback give us independently of what the device supports. For example, if the output is a stereo stream and we're playing back 5.1, we _can_ and _do_ ask OSX to give us a buffer that has six channels. This happens here: https://searchfox.org/mozilla-central/source/media/libcubeb/src/cubeb_audiounit.cpp#1303 The extraneous channels are simply dropped, but it saves some book-keeping on our side so it's worth it. ::: media/libcubeb/src/cubeb_audiounit.cpp:539 (Diff revision 1) > + > /* Get output buffer. */ > + if (stm->mixer) { > + // If remixing needs to occur, we can't directly work in our final > + // destination buffer as data may be overwritten or too small to start with. > + size_t size_needed = output_frames * stm->output_stream_params.channels * `output_frames` is available well before we're in the callback so it's quite easy to perform this allocation outside the rendering thread. ::: media/libcubeb/src/cubeb_audiounit.cpp:612 (Diff revision 1) > stm->panning.load(memory_order_relaxed) : 0.0f; > > /* Post process output samples. */ > if (stm->draining) { > /* Clear missing frames (silence) */ > memset((uint8_t*)output_buffer + outframes * outbpf, 0, (output_frames - outframes) * outbpf); Have a look at PodZero available in cubeb_utils.h, it's a bit more ergonomic. ::: media/libcubeb/src/cubeb_audiounit.cpp:613 (Diff revision 1) > > /* Post process output samples. */ > if (stm->draining) { > /* Clear missing frames (silence) */ > memset((uint8_t*)output_buffer + outframes * outbpf, 0, (output_frames - outframes) * outbpf); > + // JYA TODO : shouldn't output_frames be reduced here for the remaining panning/remix operation? If not, we'll end up mixing some silence together with some silence. Not the end of the world but easy enough. ::: media/libcubeb/src/cubeb_audiounit.cpp:2304 (Diff revision 1) > + cubeb_channel_layout_nb_channels(stm->context->layout); > + stm->output_desc.mBytesPerFrame = > + (stm->output_desc.mBitsPerChannel / 8) * > + stm->output_desc.mChannelsPerFrame; > + stm->output_desc.mBytesPerPacket = > + stm->output_desc.mBytesPerFrame * stm->output_desc.mFramesPerPacket; This should be adjusted in light of the other comment, or not if you decide to do things manually.
Attachment #8957705 -
Flags: review?(padenot)
Comment 29•6 years ago
|
||
mozreview-review |
Comment on attachment 8957706 [details] Bug 1432779 - P11. Make cubeb_mixer creation infallible. https://reviewboard.mozilla.org/r/226642/#review233196 I think this is the right behaviour to adopt indeed.
Attachment #8957706 -
Flags: review?(padenot) → review+
Comment 30•6 years ago
|
||
mozreview-review |
Comment on attachment 8957790 [details] Bug 1432779 - P12. Fix constness. https://reviewboard.mozilla.org/r/226778/#review233198
Attachment #8957790 -
Flags: review?(padenot) → review+
Assignee | ||
Comment 31•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8957704 [details] Bug 1432779 - P9. Rework cubeb_mixer. https://reviewboard.mozilla.org/r/226638/#review233134 > I don't think we can say that this is ISC and say it's derived from LGPL, but we'll clear that up. The plan discussed with :kinetik on IRC is to make this code optional, and if disabled, it will use the non-fallible case. Will amend the license for that piece of code only on a P13 > For MSVC: __popcnt > > https://docs.microsoft.com/en-us/cpp/intrinsics/popcnt16-popcnt-popcnt64 thanks, will use that. > Here is the perfect place for a comment block that explains the general strategy used here, for the matrixing. Seeing that this code is explicitly extracted from another project, I don't believe it should be up to us to document that behaviour once again. Just like you don't document how internally the resampler works. we just use it. > I think what you really want here is an assert. Real weird way of encoding this. And this pattern occurs quite often. Weird. remixing should never be called on non-sane layout. That's what there's assertion occurring in place. I have, on purpose, left this code identical to the original. I don't believe those are parts that should be rewritten. Otherwise, why bother importing code to start with? > As mentionned on irc, maybe we could drop those branches and also the `_matrix_encoding` member, since they add a fair bit of complexity in the setup. ok will remove those > This is effectively a constant in the code, and this cannot happen. I've left it open for future use where the remixer could be configured in any particular way. Yes, it will likely never happen, but i feel it's still nice to have > Aren't those branches backward ? that's ffmpeg original design... > This is a noop, `_rematrix_volume` is always 1.0 for us. same as above > I'm thinking it would be more elegant, probably more efficient and not too hard to do the dispatch on the type using templates, since your sum and copy functions are templated themselves. > > Would lead to code that is much cleaner as well. issue is that arguments passed are different and their types are different. I had looked into making this templated. But I felt this would diverge too much from the original code. The sum2 and zero are slightly different, because in the original code those were already some kind of C templates...
Assignee | ||
Comment 32•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8957704 [details] Bug 1432779 - P9. Rework cubeb_mixer. https://reviewboard.mozilla.org/r/226638/#review233134 > Seeing that this code is explicitly extracted from another project, I don't believe it should be up to us to document that behaviour once again. > Just like you don't document how internally the resampler works. we just use it. done it anyway > Shouldn't this be `FF_ARRAY_ELEMS(_matrix)`? it should yes.. thanks for spotting > issue is that arguments passed are different and their types are different. > I had looked into making this templated. > > But I felt this would diverge too much from the original code. The sum2 and zero are slightly different, because in the original code those were already some kind of C templates... I'll do it.
Assignee | ||
Comment 33•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8957704 [details] Bug 1432779 - P9. Rework cubeb_mixer. https://reviewboard.mozilla.org/r/226638/#review233134 > I've left it open for future use where the remixer could be configured in any particular way. Yes, it will likely never happen, but i feel it's still nice to have I've removed the part of the code
Assignee | ||
Comment 34•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8957705 [details] Bug 1432779 - P10. Fix remixing on mac, windows and pulse backend. https://reviewboard.mozilla.org/r/226640/#review233178 > Double check that this affirmation is correct. We're configuring the size of the buffer the callback give us independently of what the device supports. > > For example, if the output is a stereo stream and we're playing back 5.1, we _can_ and _do_ ask OSX to give us a buffer that has six channels. > > This happens here: https://searchfox.org/mozilla-central/source/media/libcubeb/src/cubeb_audiounit.cpp#1303 > > The extraneous channels are simply dropped, but it saves some book-keeping on our side so it's worth it. the request of the channels has changed. We now always request the AudioUnit output channels/layout and set the AO's input to be the same. so it is essential that we allocate a buffer for remixing. And you never know for sure the work can be done in place (though this is always true for downmixing). I feel that this optimization for a particular case of the remixing is unwarranted) We never rely on the AO to perform the remixing or make any assumptions on what it will do... > `output_frames` is available well before we're in the callback so it's quite easy to perform this allocation outside the rendering thread. yes, but this allows to guarantee that this temporary outpub_buffer is only ever accessed in the callback thread. There's no confusion nor possible races. > Have a look at PodZero available in cubeb_utils.h, it's a bit more ergonomic. output_frame is a void* though, and it can refer to either a short* or a float*.. using podzero makes it an issue as it works on typed arrays. Addtionally, this is existing code. > This should be adjusted in light of the other comment, or not if you decide to do things manually. with the oncoming change (infallability of the mixer), we end up never relying on the AudioUnit to do any mixing job. and here we set the AO's output to be the same as the AO's input. so the output types need to be adjusted accordingly
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8957790 -
Attachment is obsolete: true
Assignee | ||
Comment 46•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8957705 [details] Bug 1432779 - P10. Fix remixing on mac, windows and pulse backend. https://reviewboard.mozilla.org/r/226640/#review233178 > Is the intent here to expose this? Do we have a reason to do so? I've dropped that code.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 49•6 years ago
|
||
mozreview-review |
Comment on attachment 8957704 [details] Bug 1432779 - P9. Rework cubeb_mixer. https://reviewboard.mozilla.org/r/226638/#review233842 ::: media/libcubeb/src/cubeb_mixer.cpp:444 (Diff revision 2) > - : direction(d) > + F&& aF, uint32_t frames) > +{ > + static_assert( > + std::is_same<TYPE_COEFF, > + typename std::result_of<F(TYPE_COEFF)>::type>::value, > + "functor must return the same type as used by matrix_coeff"); This is not a functor. A functor is an object that overloads operator(). F is just a function here.
Attachment #8957704 -
Flags: review?(padenot) → review+
Comment 50•6 years ago
|
||
mozreview-review |
Comment on attachment 8957705 [details] Bug 1432779 - P10. Fix remixing on mac, windows and pulse backend. https://reviewboard.mozilla.org/r/226640/#review233844
Attachment #8957705 -
Flags: review?(padenot) → review+
Comment 51•6 years ago
|
||
mozreview-review |
Comment on attachment 8959033 [details] Bug 1432779 - P12. User proper sample size when calculating offsets. https://reviewboard.mozilla.org/r/227904/#review233846
Attachment #8959033 -
Flags: review?(padenot) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 94•6 years ago
|
||
mozreview-review |
Comment on attachment 8959257 [details] Bug 1432779 - P13. Fix input mixing and clarify frames vs samples terminology. https://reviewboard.mozilla.org/r/228122/#review234262 ::: media/libcubeb/src/cubeb_wasapi.cpp:620 (Diff revision 3) > + } else { > + if (stm->input_mixer) { > + bool ok = stm->linear_input_buffer->reserve( > + stm->linear_input_buffer->length() + input_stream_samples); > + XASSERT(ok); > + unsigned long input_packet_size = size_t ::: media/libcubeb/src/cubeb_wasapi.cpp:623 (Diff revision 3) > + stm->linear_input_buffer->length() + input_stream_samples); > + XASSERT(ok); > + unsigned long input_packet_size = > + frames * stm->input_mix_params.channels * > + cubeb_sample_size(stm->input_mix_params.format); > + unsigned long linear_input_buffer_size = size_t
Attachment #8959257 -
Flags: review?(padenot) → review+
Comment 95•6 years ago
|
||
mozreview-review |
Comment on attachment 8959258 [details] Bug 1432779 - P14. If a layout is unknown or invalid, always treat it as plain stereo or mono. https://reviewboard.mozilla.org/r/228124/#review234264
Attachment #8959258 -
Flags: review?(padenot) → review+
Comment 96•6 years ago
|
||
mozreview-review |
Comment on attachment 8959258 [details] Bug 1432779 - P14. If a layout is unknown or invalid, always treat it as plain stereo or mono. https://reviewboard.mozilla.org/r/228124/#review234278
Assignee | ||
Comment 97•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8959257 [details] Bug 1432779 - P13. Fix input mixing and clarify frames vs samples terminology. https://reviewboard.mozilla.org/r/228122/#review234262 > size_t i only put the code that i removed in P9 back > size_t same
Assignee | ||
Comment 98•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8959257 [details] Bug 1432779 - P13. Fix input mixing and clarify frames vs samples terminology. https://reviewboard.mozilla.org/r/228122/#review234262 > i only put the code that i removed in P9 back while at it, I replaced on P9 all unsigned long with size_t
Comment 99•6 years ago
|
||
Pushed by jyavenard@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bca1660fc65c Update libcubeb to revision 77cb1a9. r=padenot https://hg.mozilla.org/integration/mozilla-inbound/rev/b98af64fa15c Update cubeb-pulse-rs to commit 247b01d. r=jya https://hg.mozilla.org/integration/mozilla-inbound/rev/2bbfded200e0 Rust vendor. r=jya https://hg.mozilla.org/integration/mozilla-inbound/rev/0b1a55a9b166 Update audioipc to commit 7b3af898. r=kamidphish https://hg.mozilla.org/integration/mozilla-inbound/rev/355c35d358fb P1. Fix ChannelLayout calculation for uncommon layouts. r=padenot https://hg.mozilla.org/integration/mozilla-inbound/rev/075811af8d4a P3. Properly set channel map layout after seeking. r=padenot https://hg.mozilla.org/integration/mozilla-inbound/rev/4e8af01f19c2 P4. Remove the concept of preferred layout. r=padenot,r=kamidphish https://hg.mozilla.org/integration/mozilla-inbound/rev/52a4fb079d6c P5. Remove no longer used variables. r=padenot https://hg.mozilla.org/integration/mozilla-inbound/rev/d3d5dc721d47 P7. Use typedef rather than actual type. r=padenot
Comment 100•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bca1660fc65c https://hg.mozilla.org/mozilla-central/rev/b98af64fa15c https://hg.mozilla.org/mozilla-central/rev/2bbfded200e0 https://hg.mozilla.org/mozilla-central/rev/0b1a55a9b166 https://hg.mozilla.org/mozilla-central/rev/355c35d358fb https://hg.mozilla.org/mozilla-central/rev/075811af8d4a https://hg.mozilla.org/mozilla-central/rev/4e8af01f19c2 https://hg.mozilla.org/mozilla-central/rev/52a4fb079d6c https://hg.mozilla.org/mozilla-central/rev/d3d5dc721d47
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•