Closed Bug 1444479 Opened 2 years ago Closed 2 years ago

Remove decoder channel limit restrictions.

Categories

(Core :: Audio/Video: Playback, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(11 files)

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
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
59 bytes, text/x-review-board-request
padenot
: review+
Details
Following bug 1432779 and bug 1431221, we no longer need to be limited to 8 decoded audio channels.

We will remove all limits and let each decoder and cubeb deal with it...
Priority: -- → P3
Comment on attachment 8959583 [details]
Bug 1444479 - P1. Remove 8 channels limitation in AudioConfig.

https://reviewboard.mozilla.org/r/228384/#review234288
Attachment #8959583 - Flags: review?(padenot) → review+
Comment on attachment 8959584 [details]
Bug 1444479 - P2. Make AudioConverter works with unknown layout.

https://reviewboard.mozilla.org/r/228386/#review234280

::: dom/media/AudioConverter.cpp:148
(Diff revision 1)
>    return aX < -32768 ? -32768 : aX <= 32767 ? aX : 32767;
>  }
>  
> +template<typename TYPE>
> +static void
> +dumbCopy(TYPE* aOut,

Maybe this could have a name that is a bit more explicit.
Attachment #8959584 - Flags: review?(padenot) → review+
Comment on attachment 8959585 [details]
Bug 1444479 - P3. Add Channels(ChannelMap) method.

https://reviewboard.mozilla.org/r/228388/#review234282

::: dom/media/AudioConfig.h:121
(Diff revision 1)
> +      aMap -= (aMap >> 1) & 0x55555555;
> +      aMap = (aMap & 0x33333333) + ((aMap >> 2) & 0x33333333);
> +      aMap = (aMap + (aMap >> 4)) & 0x0F0F0F0F;
> +      aMap += aMap >> 8;
> +      return (aMap + (aMap >> 16)) & 0x3F;
> +#endif

Use https://searchfox.org/mozilla-central/source/mfbt/MathAlgorithms.h#338
Attachment #8959585 - Flags: review?(padenot) → review+
Comment on attachment 8959586 [details]
Bug 1444479 - P4. Add new AudioConfig constructor.

https://reviewboard.mozilla.org/r/228390/#review234290
Attachment #8959586 - Flags: review?(padenot) → review+
Comment on attachment 8959587 [details]
Bug 1444479 - P5. Let AudioSink deal with unknown layout.

https://reviewboard.mozilla.org/r/228392/#review234292
Attachment #8959587 - Flags: review?(padenot) → review+
Comment on attachment 8959588 [details]
Bug 1444479 - P6. Make Opus and Vorbis decoder deal with more channels than 8.

https://reviewboard.mozilla.org/r/228394/#review234284

::: commit-message-b1c55:4
(Diff revision 1)
> +Bug 1444479 - P6. Make Opus and Vorbis decoder deal with more channels than 8. r?padenot
> +
> +Under 8 channels, the audio will be reordered so it can be playable on any platforms.
> +Over 8 channels, the channels will be as output by the decoder. Playing of such stream will be platform dependent as neither Opus nor Vorbis define a channel layout with more than 8 channels.

Fix the message so that it's clear what happens at exactly 8 channels (strictly over vs. over).

::: dom/media/ogg/OpusParser.cpp:84
(Diff revision 1)
>        mCoupledStreams = mChannels - 1;
>        mMappingTable[0] = 0;
>        mMappingTable[1] = 1;
>      } else if (mChannelMapping == 1 || mChannelMapping == 255) {
>        // Currently only up to 8 channels are defined for mapping family 1 and we
>        // only supports only up to 8 channels for mapping family 255.

I think the comment needs an update: we now support more channels when the mapping family _is_ 255.
Attachment #8959588 - Flags: review?(padenot) → review+
Comment on attachment 8959589 [details]
Bug 1444479 - P7. Allow Apple AudioToolbox decoder handle more than 8 channels.

https://reviewboard.mozilla.org/r/228396/#review234296
Attachment #8959589 - Flags: review?(padenot) → review+
Comment on attachment 8959590 [details]
Bug 1444479 - P8. Remove FFmpeg audio decoder channel limitation.

https://reviewboard.mozilla.org/r/228398/#review234298
Attachment #8959590 - Flags: review?(padenot) → review+
Comment on attachment 8959591 [details]
Bug 1444479 - P9. Remove WMF audio decoder channel limit.

https://reviewboard.mozilla.org/r/228400/#review234302
Attachment #8959591 - Flags: review?(padenot) → review+
Comment on attachment 8959592 [details]
Bug 1444479 - P10. Don't reject files with more than 8 audio channels.

https://reviewboard.mozilla.org/r/228402/#review234304
Attachment #8959592 - Flags: review?(padenot) → review+
Comment on attachment 8959592 [details]
Bug 1444479 - P10. Don't reject files with more than 8 audio channels.

https://reviewboard.mozilla.org/r/228402/#review234306
Comment on attachment 8959593 [details]
Bug 1444479 - P11. Remove ununsed constant.

https://reviewboard.mozilla.org/r/228404/#review234308
Attachment #8959593 - Flags: review?(padenot) → review+
Comment on attachment 8959585 [details]
Bug 1444479 - P3. Add Channels(ChannelMap) method.

https://reviewboard.mozilla.org/r/228388/#review234626

::: dom/media/AudioConfig.h:121
(Diff revision 1)
> +      aMap -= (aMap >> 1) & 0x55555555;
> +      aMap = (aMap & 0x33333333) + ((aMap >> 2) & 0x33333333);
> +      aMap = (aMap + (aMap >> 4)) & 0x0F0F0F0F;
> +      aMap += aMap >> 8;
> +      return (aMap + (aMap >> 16)) & 0x3F;
> +#endif

thanks.. learnt something new today...
Comment on attachment 8959588 [details]
Bug 1444479 - P6. Make Opus and Vorbis decoder deal with more channels than 8.

https://reviewboard.mozilla.org/r/228394/#review234284

> Fix the message so that it's clear what happens at exactly 8 channels (strictly over vs. over).

Suggestions welcome, I don't see how I can make this comment any clearer.
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/63b5bd67f700
P1. Remove 8 channels limitation in AudioConfig. r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ea728f4e688
P2. Make AudioConverter works with unknown layout. r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/b821de55844f
P3. Add Channels(ChannelMap) method. r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e3da7510ef1
P4. Add new AudioConfig constructor. r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/3207ab7618d5
P5. Let AudioSink deal with unknown layout. r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/938c300a46ca
P6. Make Opus and Vorbis decoder deal with more channels than 8. r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/6531087bc206
P7. Allow Apple AudioToolbox decoder handle more than 8 channels. r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3ff23419244
P8. Remove FFmpeg audio decoder channel limitation. r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef58e2386804
P9. Remove WMF audio decoder channel limit. r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/34d5728973b4
P10. Don't reject files with more than 8 audio channels. r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d4c3cf648d6
P11. Remove unused constant. r=padenot
You need to log in before you can comment on or make changes to this bug.