Closed Bug 1431221 Opened 3 years ago Closed 2 years ago

4.0 audio file doesn't play properly

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(15 files)

1.64 MB, video/mp4
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
cpearce
: 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
cpearce
: 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
Attached video ChID-BLITS-EBU-4.mp4
The channel placement appears totally rubbish on mac at least...

The attached file was generated from https://www2.iis.fraunhofer.de/AAC/ChID-BLITS-EBU.mp4 using ffmpeg

ffmpeg -i ChID-BLITS-EBU.mp4 -c:v copy -ac 4 ChID-BLITS-EBU-4.mp4

This file doesn't play without bug 1431169 applied.

When playing, we can only hear the left and right channel. center and surround are gone (there's no LFE channel)

Left is on the Right
Center (which should be stereo) is Left
no other channels can be heard.
This is a cubeb issue, as AudioConverter isn't used
Component: Audio/Video: Playback → Audio/Video: cubeb
Flags: needinfo?(cchang)
it's also bad on windows.
Only left and right channels can be heard, all the other are silent
according to the mac AAC decoder, this 4.0 is made of:
Front Left + Front Center + Front Right + Read Surround

the AudioLayout however assumes that the SMPTE default is owever, assume that 4.0 default is https://searchfox.org/mozilla-central/source/dom/media/MediaInfo.cpp#91

there's no logic to convert the layout to another at this stage, and as such the channels are lost.
Assignee: nobody → jyavenard
Blocks: 1378070
Keywords: regression
No longer blocks: 1378070
Keywords: regression
Rank: 25
Priority: -- → P3
Flags: needinfo?(chun.m.chang)
Blocks: 1432779
Comment on attachment 8947381 [details]
Bug 1431221 - P9. Properly retrieve and set channel layout on windows.

https://reviewboard.mozilla.org/r/217108/#review223006

::: dom/media/platforms/wmf/WMFAudioMFTManager.cpp:347
(Diff revision 2)
>                             duration,
>                             numFrames,
>                             Move(audioData),
>                             mAudioChannels,
> -                           mAudioRate);
> +                           mAudioRate,
> +						   mChannelsMap);

drive-by: indent is a bit off here.
Comment on attachment 8944697 [details]
Bug 1431221 - P1. Add method to retrieve SMPTE equivalent layout from any channel layout.

https://reviewboard.mozilla.org/r/214844/#review223040

::: dom/media/MediaInfo.cpp:240
(Diff revision 3)
> +    case L3F2_MAP: return L3F2;
> +    case L3F2_LFE_MAP: return L3F2_LFE;
> +    case L3F3R_LFE_MAP: return L3F3R_LFE;
> +    case L3F4_LFE_MAP: return L3F4_LFE;
> +    default:
> +      // unknown return identical.

No log/assert/something here?
Attachment #8944697 - Flags: review?(padenot) → review+
Comment on attachment 8944698 [details]
Bug 1431221 - P2. Have mac AAC decoder use proper output layout.

https://reviewboard.mozilla.org/r/214846/#review223044
Attachment #8944698 - Flags: review?(padenot) → review+
Comment on attachment 8944699 [details]
Bug 1431221 - P6. Add channel map information to AudioInfo.

https://reviewboard.mozilla.org/r/214848/#review223004

::: dom/media/MediaData.h:391
(Diff revision 3)
>              const media::TimeUnit& aDuration,
>              uint32_t aFrames,
>              AlignedAudioBuffer&& aData,
>              uint32_t aChannels,
> -            uint32_t aRate)
> +            uint32_t aRate,
> +            uint32_t aChannelMap = 0)

Maybe a type that is a bit more descriptive here?

::: dom/media/MediaData.h:423
(Diff revision 3)
>    // the audiable data and silent data.
>    bool IsAudible() const;
>  
>    const uint32_t mChannels;
> +  // The AudioConfig::ChannelLayout map. Channels are ordered as per SMPTE
> +  // definition. A value of 0 indicates unknown layout.

I don't like this kind of stuff. I you want to refer to something that is invalid, put a name on it that say it's invalid.
Attachment #8944699 - Flags: review?(padenot) → review-
Comment on attachment 8944700 [details]
Bug 1431221 - P4. Use similar channel decriptions as Windows WAVE and FFmpeg.

https://reviewboard.mozilla.org/r/214850/#review223048
Attachment #8944700 - Flags: review?(padenot) → review+
Comment on attachment 8944701 [details]
Bug 1431221 - P7. Pass channel layout on mac.

https://reviewboard.mozilla.org/r/214852/#review223050

r+ in principles, granted you've fixed this issue about passing in constant literal at call site in the other patch.

::: dom/media/platforms/apple/AppleATDecoder.cpp:338
(Diff revision 3)
> -                                          duration,
> +                  duration,
> -                                          numFrames,
> +                  numFrames,
> -                                          data.Forget(),
> +                  data.Forget(),
> -                                          channels,
> +                  channels,
> -                                          rate);
> +                  rate,
> +                  mChannelLayout ? mChannelLayout->Map() : 0);

Don't like a "0" at call site like this either, it's not super readable.
Attachment #8944701 - Flags: review?(padenot) → review+
Comment on attachment 8944702 [details]
Bug 1431221 - P8. Pass channel layout with FFmpeg.

https://reviewboard.mozilla.org/r/214854/#review223052

::: dom/media/platforms/ffmpeg/FFmpegAudioDecoder.cpp:183
(Diff revision 3)
>  #endif
>  
>    return audio;
>  }
>  
> +typedef AudioConfig::ChannelLayout ChannelLayout;

Is that needed?
Attachment #8944702 - Flags: review?(padenot) → review+
Comment on attachment 8944703 [details]
Bug 1431221 - P3. Fix unified build.

https://reviewboard.mozilla.org/r/214856/#review223054

::: dom/media/MediaInfo.cpp:208
(Diff revision 3)
>  
> -/* static */ const AudioConfig::ChannelLayout&
> +/* static */ AudioConfig::ChannelLayout
>  AudioConfig::ChannelLayout::SMPTEDefault(
>    const ChannelLayout& aChannelLayout)
>  {
> +  if (!aChannelLayout.IsValid()) {

Maybe we could signal that something went wrong with an NS_WARNING or other mechanism? Often helps in the field when an ISV is trying to debug things that don't work.

::: dom/media/MediaInfo.cpp:257
(Diff revision 3)
>      case L3F3R_LFE_MAP: return L3F3R_LFE;
>      case L3F4_LFE_MAP: return L3F4_LFE;
>      default:
> -      // unknown return identical.
> -      return aChannelLayout;
> +      break;
> +  }
> +  AutoTArray<Channel, MAX_AUDIO_CHANNELS> layout;

I'd have expected a comment here to answer to the "first handle the most common case" above, something like: "else remap to SMPTE ordering" or what not.
Attachment #8944703 - Flags: review?(padenot) → review+
Comment on attachment 8947382 [details]
Bug 1431221 - P5. Split AudioConfig.{h,cpp} from MediaInfo.

https://reviewboard.mozilla.org/r/217110/#review223056

Cool. Remember to land the right bits in the right repo. I can help with having a quick turnaround for cubeb in europe.
Attachment #8947382 - Flags: review?(padenot) → review+
Comment on attachment 8947383 [details]
Bug 1431221 - P13. Remove dual mono layout.

https://reviewboard.mozilla.org/r/217112/#review223058
Attachment #8947383 - Flags: review?(padenot) → review+
Comment on attachment 8944699 [details]
Bug 1431221 - P6. Add channel map information to AudioInfo.

https://reviewboard.mozilla.org/r/214848/#review223004

> Maybe a type that is a bit more descriptive here?

This must not be aa type. It's a bit mask. 0 doesn't mean invalid it means unknown. There's just no mapping then. This is the same value as used by most audio decoders as well as the playback backend.

> I don't like this kind of stuff. I you want to refer to something that is invalid, put a name on it that say it's invalid.

It is not invalid. Having no layout is fine and legal. We just don't know. The channel map is a
Comment on attachment 8947381 [details]
Bug 1431221 - P9. Properly retrieve and set channel layout on windows.

https://reviewboard.mozilla.org/r/217108/#review223144
Attachment #8947381 - Flags: review?(cpearce) → review+
Comment on attachment 8947394 [details]
Bug 1431221 - P10. Properly retrieve and set channel layout for opus and vorbis.

https://reviewboard.mozilla.org/r/217114/#review223146
Attachment #8947394 - Flags: review?(cpearce) → review+
Blocks: 1434362
Blocks: 1435598
Comment on attachment 8944697 [details]
Bug 1431221 - P1. Add method to retrieve SMPTE equivalent layout from any channel layout.

https://reviewboard.mozilla.org/r/214844/#review223040

> No log/assert/something here?

i don't see the advantage in doing so, cubeb will ultimately be responsible in doing so
Comment on attachment 8944702 [details]
Bug 1431221 - P8. Pass channel layout with FFmpeg.

https://reviewboard.mozilla.org/r/214854/#review223052

> Is that needed?

makes formatting easier , hard to make anything fit on 80 columns when the scope is so long to start with
Comment on attachment 8944699 [details]
Bug 1431221 - P6. Add channel map information to AudioInfo.

https://reviewboard.mozilla.org/r/214848/#review225556
Attachment #8944699 - Flags: review?(padenot) → review+
Comment on attachment 8950355 [details]
Bug 1431221 - P12. Fix Force Stereo Mode.

https://reviewboard.mozilla.org/r/219578/#review225560
Attachment #8950355 - Flags: review?(padenot) → review+
Comment on attachment 8950354 [details]
Bug 1431221 - P11. Configure audio to use desired channel layout.

https://reviewboard.mozilla.org/r/219576/#review225558
Attachment #8950354 - Flags: review?(padenot) → review+
Blocks: 1444479
Comment on attachment 8961867 [details]
Bug 1431221 - P14. Remove static layout definitions.

https://reviewboard.mozilla.org/r/230694/#review236172
Attachment #8961867 - Flags: review?(padenot) → review+
Comment on attachment 8944697 [details]
Bug 1431221 - P1. Add method to retrieve SMPTE equivalent layout from any channel layout.

https://reviewboard.mozilla.org/r/214844/#review236176


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/MediaInfo.h:654
(Diff revision 5)
>          return;
>        }
>        mChannels.AppendElements(aConfig, aChannels);
>        UpdateChannelMap();
>      }
> +    ChannelLayout(std::initializer_list<Channel> aChannelList)

Error: Bad implicit conversion constructor for 'channellayout' [clang-tidy: mozilla-implicit-constructor]
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14d29c07af27
P1. Add method to retrieve SMPTE equivalent layout from any channel layout. r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/35d2f3b44644
P2. Have mac AAC decoder use proper output layout. r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcf9d1e427a0
P3. Fix unified build. r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd673186a4ef
P4. Use similar channel decriptions as Windows WAVE and FFmpeg. r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c8f7cf1be79
P5. Split AudioConfig.{h,cpp} from MediaInfo. r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2450584957a
P6. Add channel map information to AudioInfo. r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa33cb272d1b
P7. Pass channel layout on mac. r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/20ba26621d34
P8. Pass channel layout with FFmpeg. r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/0abb67153626
P9. Properly retrieve and set channel layout on windows. r=cpearce
https://hg.mozilla.org/integration/mozilla-inbound/rev/4138a501de01
P10. Properly retrieve and set channel layout for opus and vorbis. r=cpearce
https://hg.mozilla.org/integration/mozilla-inbound/rev/efe8e71bf37b
P11. Configure audio to use desired channel layout. r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/b16f182e5bcc
P12. Fix Force Stereo Mode. r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/25a9b9ad0e6b
P13. Remove dual mono layout. r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/20b381d4bfc1
P14. Remove static layout definitions. r=padenot
You need to log in before you can comment on or make changes to this bug.