Refactor PCM format handling to use Util methods#3063
Refactor PCM format handling to use Util methods#3063copybara-service[bot] merged 2 commits intoandroidx:mainfrom
Conversation
nift4
force-pushed
the
pcmftidyup
branch
2 times, most recently
from
8b4e6c0 to
b5e492f
Compare
rohitjoins
force-pushed
the
pcmftidyup
branch
from
b5e492f to
c8c9cac
Compare
|
Hi @nift4, Thank you for raising this PR! On a closer look, it seems that this PR is actually doing quite a few different things at once. The current commit summary "Tidy up pcm format handling in some places" doesn't quite do justice to changes you've included here. To make this clean and easy to review, could I ask you to please split this up into smaller, separate PRs that handle one logical change at a time (not necessarily in this order)?
This will make it much easier for us to safely review and merge each independent change. Thank you! Also, please feel free to assign these PRs directly to me. |
|
Hi @rohitjoins, thanks for the quick response. It was done this way because the individual parts are quite small and conflict with each other, but I understand it's harder to review this way. I split it up into three parts as you said: The refactor is included in this PR, the bug fix for 20-bit files is #3089 and the feature one is #3090. |
| } | ||
| } else if (bitsPerSample == 32) { | ||
| pcmEncoding = Util.getPcmEncoding(bitsPerSample, isBigEndian ? BIG_ENDIAN : LITTLE_ENDIAN); | ||
| } else if (!isBigEndian && bitsPerSample == 32) { |
There was a problem hiding this comment.
Addition of !isBigEndian looks more like a change in logic than pure refactor, was it intentional?
There was a problem hiding this comment.
Oh, my bad I forgot to split that. It's a bug fix because ENCODING_PCM_FLOAT refers to little endian only and big endian would cause static noise playback. Should we merge this here or in another PR?
There was a problem hiding this comment.
Thanks for clarifying! Since it changes behavior, it would ideally be in its own PR. But if creating another one is a pain, no worries at all, I can take care of splitting this change internally on our end. Let me know!
There was a problem hiding this comment.
Because I'll be away from my computer for a few days, it'd probably be best if you split this change internally instead. Thank you!
- refactor a few usages that manually handle all pcm formats to the Util methods that already exist.
rohitjoins
force-pushed
the
pcmftidyup
branch
from
8ccd1ce to
ca61f3f
Compare
rohitjoins
changed the title
This change adds an explicit endianness check (`!isBigEndian`) when parsing the audio sample entry in `BoxParser.java` to prevent mapping big-endian 32-bit float audio to a little-endian encoding, avoiding static noise during playback. Issue: #3063 PiperOrigin-RevId: 875133592
2 participants
Tidy up PCM format handling by refactoring a few usages that manually handle all PCM formats to use the Util methods that already exist.
This also adds missing string representations for existing formats (
C.ENCODING_PCM_DOUBLEandC.ENCODING_DSD) toEventLoggerto complete format coverage.This is a pure refactoring change and does not alter existing playback behavior.