Update Bs1770gainBackend to handle bs1770gain v0.6.0 XML output by ybnd · Pull Request #3480 · beetbox/beets

ybnd · 2020-01-30T16:06:59Z

* Remove `0%\x08\x08` from output (backspace code doesn't resolve; progress percentages get spliced in) * Handle changed attributes/fields: * `sample-peak` attribute `factor` is called `amplitude` instead * Album summary is not included in a `summary` tag now, but in two separate `integrated` and `sample-peak` tags * Handle `lu` attribute * Get bs1770gain version * If v0.6.0 or later, add `--unit=ebu` flag to convert `db` attributes to LUFS * May be useful later on ### Output examples Track: ```  <bs1770gain norm="-18.00"> <track total="1" number="1" file="02 tïtle 0.mp3"> <integrated lufs="-70.00" lu="52.00"/> <sample-peak spfs="-72.28" amplitude="0.00"/> </track> <integrated lufs="-70.00" lu="52.00"/> <sample-peak spfs="-72.28" amplitude="0.00"/> </bs1770gain>  ``` Album: ```  <bs1770gain norm="-18.00"> <track total="2" number="1" file="02 tïtle 0.mp3"> <integrated dbfs="-70.00" db="52.00"/> <sample-peak dbfs="-72.28" amplitude="0.00"/> </track> <track total="2" number="2" file="02 tïtle 1.mp3"> <integrated dbfs="-70.00" db="52.00"/> <sample-peak dbfs="-72.28" amplitude="0.00"/> </track> <integrated dbfs="-70.00" db="52.00"/> <sample-peak dbfs="-72.28" amplitude="0.00"/> </bs1770gain>  ```

* Fix unspecified `gain_adjustment` when method defined in config * Fix difference between dB and LUFS values in case of mismatched `target_level`/`method`: ``` db_to_lufs( target_level <dB> ) - lufs_to_dB( -23 <LUFS> ) ``` * Ignore single assertion in case of bs1770gain (cherry picked from commit 2395bf2)

* With `bs1770gain` installed the `Bs1770gainBackend` tests fail, but this should be fixed by beetbox#3480.

sampsyo

Cool; thanks for attacking this! Here are a few ideas.

beetsplug/replaygain.py

sampsyo · 2020-01-30T16:59:07Z

beetsplug/replaygain.py

+                    '([0-9]+.[0-9]+.[0-9]+), ',
+                    call([cmd, '--version']).stdout.decode('utf-8')
+                ).group(1)
+            except AttributeError:


I’m a little confused—what is the potential AttributeError here? Is it the stdout attribute of the call result?

Maybe it would be nicer to just do a single call to check both whether the command exists and to get its version. Then we’d just need to handle OSError, I think?

Yes, it's the stdout attribute.
Ok, I suppose there's no real chance that some weird version bs1770gain doesn't handle --version.

sampsyo · 2020-01-30T17:01:23Z

beetsplug/replaygain.py

+            if type(text) == bytes:
+                text = text.decode('utf-8')
+            while '\x08' in text:
+                text = re.sub('[^\x08]\x08', '', text)


This replacement is somewhat surprising! Maybe a comment would be useful here to describe what’s going wrong?

sampsyo · 2020-01-30T17:01:47Z

beetsplug/replaygain.py

        parser.EndElementHandler = end_element_handler

        try:
+            if type(text) == bytes:


Suggested change

if type(text) == bytes:

if isinstance(text, bytes):

sampsyo · 2020-01-30T17:04:35Z

beetsplug/replaygain.py


        try:
+            if type(text) == bytes:
+                text = text.decode('utf-8')


Is it necessary to do this conversion, or shall we perhaps just do the substitution directly on the bytes (with a bytes pattern)? That would avoid hard-coding an encoding, for example...

sampsyo · 2020-01-30T17:05:27Z

beetsplug/replaygain.py

        try:
+            if type(text) == bytes:
+                text = text.decode('utf-8')
+            while '\x08' in text:


It might be safer to loop until the string stops changing. Otherwise, this runs the risk of this character being present but the pattern not matching (e.g., if it occurs at the beginning of the string).

To me it seems more readable to cover that case in the pattern by also replacing \x08 at the start of the string

b'[^\x08]\x08|^\x08'

then I can just explain it in a comment since I should write a bit about what's going on here anyway (:

* safer version comparison * regex bytes directly * handle b'\x08 ...' case * test_replaygain.py: injected command output should match the type of the actual output

sampsyo

Looking good! I have a couple of questions within, but I think this is basically ready to go. Would you mind adding a quick changelog entry?

sampsyo · 2020-01-31T00:22:33Z

beetsplug/replaygain.py

            raise FatalReplayGainError(
                u'Is bs1770gain installed?'
            )
+        except AttributeError:


Thanks for the reorganization, but I'm still not 100% sure when this exception can be raised. Isn't it the case that the command_output object will always have a stdout member if the call was successful?

There is an edge case in test_replaygain where the backend is tested while bypassing calls to bs1770gain.
It was patching None to stdout there, I made it patch bs1770gain 0.0.0, instead.

beetsplug/replaygain.py

…arsing # Conflicts: # docs/changelog.rst

sampsyo

Looks lovely; thank you for your persistence!

ybnd added 2 commits January 30, 2020 16:13

Don't call bs1770gain outside of try statement

c78afb1

ybnd changed the title ~~Bs1770gain v0.6.0 parsing~~ Update Bs1770gainBackend parsing to handle bs1770gain v0.6.0 XML output Jan 30, 2020

ybnd changed the title ~~Update Bs1770gainBackend parsing to handle bs1770gain v0.6.0 XML output~~ Update Bs1770gainBackend to handle bs1770gain v0.6.0 XML output Jan 30, 2020

ybnd added a commit to ybnd/beets that referenced this pull request Jan 30, 2020

Exception handling in main & worker threads

65ffca2

* With `bs1770gain` installed the `Bs1770gainBackend` tests fail, but this should be fixed by beetbox#3480.

sampsyo reviewed Jan 30, 2020

View reviewed changes

ybnd added 2 commits January 30, 2020 19:21

Implement review comments

c3817a4

* safer version comparison * regex bytes directly * handle b'\x08 ...' case * test_replaygain.py: injected command output should match the type of the actual output

Remove packaging dependency

506be02

sampsyo reviewed Jan 31, 2020

View reviewed changes

ybnd added 5 commits January 31, 2020 07:42

Remove spliced progress regex and add --suppress-progress flag

bef473c

Add to changelog

9465e93

Merge remote-tracking branch 'origin/master' into bs1770gain-v0.6.0-p…

1ab46c6

…arsing # Conflicts: # docs/changelog.rst

Add comment to clarify unexpected AttributeError handling

7005691

Modify patched stdout in test_malformed_output

63ea173

ybnd requested a review from sampsyo February 5, 2020 08:07

sampsyo approved these changes Feb 5, 2020

View reviewed changes

sampsyo merged commit 817bef2 into beetbox:master Feb 5, 2020

sampsyo mentioned this pull request Sep 1, 2020

bs1770gain 0.7.0 compatibility #3740

Closed

doronbehar mentioned this pull request Sep 11, 2020

bs1770gain: 0.5.2 -> 0.7.0 NixOS/nixpkgs#90504

Closed

Conversation

ybnd commented Jan 30, 2020

Uh oh!

sampsyo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sampsyo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sampsyo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants