Pass AVChannelLayout to ChannelLayoutToChromeChannelLayout [chromium/src : main]
Thomas Guilbert (Gerrit)
Thomas Guilbert added 1 comment![]( "Open in Gerrit")
if (layout.order == AV_CHANNEL_ORDER_AMBISONIC) {
return CHANNEL_LAYOUT_DISCRETE;
}
To be extra, extra cautious, we could split the changes in 2:
1) Pass AVChannelLayout here (no-op) and add a UMA for Ambisonic
2) Force Ambisonic to DISCRETE.
Related details
- Syed AbuTalib
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Syed AbuTalib (Gerrit)
Syed AbuTalib added 1 comment![]( "Open in Gerrit")
if (layout.order == AV_CHANNEL_ORDER_AMBISONIC) {
return CHANNEL_LAYOUT_DISCRETE;
}
To be extra, extra cautious, we could split the changes in 2:
1) Pass AVChannelLayout here (no-op) and add a UMA for Ambisonic
2) Force Ambisonic to DISCRETE.
Made this CL a no-op, will send the next CL with a `DO NOT SUBMIT` comment.
Related details
- Thomas Guilbert
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Guilbert (Gerrit)
Thomas Guilbert added 4 comments![]( "Open in Gerrit")
This change prevents a bug where an 11-channel stream (such as a 2nd
order ambisonic layout + stereo) would be incorrectly
identified as a standard stereo layout if the maximum channel limit is
increased. By checking the layout order, the decoder can correctly
identify these cases as discrete layouts.
Additionally, this patch:
* Includes a new UMA histogram, Media.Ambisonic.ChannelCount, to track
the frequency and channel counts of ambisonic streams.
* Simplifies call sites by passing the internal FFmpeg layout directly.Too much detail, you can remove this.
<histogram name="Media.Ambisonic.ChannelCount" units="units"I would put this in `Media.Audio.` at least. Possibly `Media.Audio.Layouts.Ambisonic.ChannelCount`?
<histogram name="Media.Ambisonic.ChannelCount" units="units"Channels
Channel count of Ambisonic layouts as determined by FFmpeg. This is to track
how often 1st order, 2nd, 3rd, etc. layouts are seen in media.You should specify when this is logged.
Related details
- Syed AbuTalib
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Syed AbuTalib (Gerrit)
Syed AbuTalib added 4 comments![]( "Open in Gerrit")
This change prevents a bug where an 11-channel stream (such as a 2nd
order ambisonic layout + stereo) would be incorrectly
identified as a standard stereo layout if the maximum channel limit is
increased. By checking the layout order, the decoder can correctly
identify these cases as discrete layouts.
Additionally, this patch:
* Includes a new UMA histogram, Media.Ambisonic.ChannelCount, to track
the frequency and channel counts of ambisonic streams.
* Simplifies call sites by passing the internal FFmpeg layout directly.Too much detail, you can remove this.
Done
<histogram name="Media.Ambisonic.ChannelCount" units="units"Syed AbuTalibChannels
Done
<histogram name="Media.Ambisonic.ChannelCount" units="units"I would put this in `Media.Audio.` at least. Possibly `Media.Audio.Layouts.Ambisonic.ChannelCount`?
Done
Channel count of Ambisonic layouts as determined by FFmpeg. This is to track
how often 1st order, 2nd, 3rd, etc. layouts are seen in media.You should specify when this is logged.
- Thomas Guilbert
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Guilbert (Gerrit)
Thomas Guilbert added 3 comments![]( "Open in Gerrit")
base::UmaHistogramExactLinear("Media.Ambisonic.ChannelCount",This needs to match the path in the histogram XML.
layout.nb_channels, 33);Use
```
constexpr int kMaxAmbisonicsChannels = 32;
static_assert(kMaxAmbisonicsChannels == media::limits::kMaxChannels, "kMaxAmbisonicsChannels doesn't match kMaxChannels")
base::UmaHistogramExactLinear(..., kMaxAmbisonicsChannels+1);
```
so we get a warning if ever kMaxChannel was bumped up (I don't think UMA would like). This would need to be handled at that time.
Related details
- Syed AbuTalib
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Syed AbuTalib (Gerrit)
Syed AbuTalib added 2 comments![]( "Open in Gerrit")
base::UmaHistogramExactLinear("Media.Ambisonic.ChannelCount",This needs to match the path in the histogram XML.
Dang, surprised I missed this. Done.
Use
```
constexpr int kMaxAmbisonicsChannels = 32;
static_assert(kMaxAmbisonicsChannels == media::limits::kMaxChannels, "kMaxAmbisonicsChannels doesn't match kMaxChannels")
base::UmaHistogramExactLinear(..., kMaxAmbisonicsChannels+1);
```so we get a warning if ever kMaxChannel was bumped up (I don't think UMA would like). This would need to be handled at that time.
- Thomas Guilbert
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |