iluvcapra / wavinfo

Probe WAVE Files for all metadata
https://wavinfo.readthedocs.io/
MIT License
36 stars 7 forks source link

Update rf64_parser.py #31

Closed dannyniu closed 11 months ago

dannyniu commented 11 months ago

Fix a error with formula used in 64-bit chunk size table offset.

codecov[bot] commented 11 months ago

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (067cca8) 95.70% compared to head (ffbe908) 95.63%.

:exclamation: Current head ffbe908 differs from pull request most recent head aace7b9. Consider uploading reports for the commit aace7b9 to get more accurate results

Files Patch % Lines
wavinfo/rf64_parser.py 66.66% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #31 +/- ## ========================================== - Coverage 95.70% 95.63% -0.08% ========================================== Files 22 22 Lines 1303 1305 +2 ========================================== + Hits 1247 1248 +1 - Misses 56 57 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

iluvcapra commented 11 months ago

I've turned on tests for this branch, thank you for the addition. Can you please provide a test WAV file that exercises this new code?

dannyniu commented 11 months ago

While I'm certain that the change is correct, I'm not able to provide any test vector for it. This is because the data chunk is likely the ONLY chunk in the file that need 64-bit sizes.

I could try make a Dolby Atmos master file with gigantic ADM, but it'll probably be too difficult to send it to you.

dannyniu commented 11 months ago

I added a commit to fix a typo. Should be able to pass linting with flake8. But again, my changes are difficult to test because it involve insanely large test vectors.

iluvcapra commented 11 months ago

I added a commit to fix a typo. Should be able to pass linting with flake8. But again, my changes are difficult to test because it involve insanely large test vectors.

I would create a silent test wav flle and then add it as a .gz file to the tests/test_files/rf64 directory. At least tests/test_rf64.py will hit it and open it, that should cover the code you've added.

dannyniu commented 11 months ago

I would create a silent test wav flle

No use, because we need to test larger-than-4GiB chunks other than data, which is what my modification all about. But to do that, I need to produce a non-data chunk that's larger than 4GiB, which will be too diffcult to compress and upload.

dannyniu commented 11 months ago

@iluvcapra Is it possible that I simply upload a big file as dummy, so that coverage test will pass even though the newly added code path wasn't reached?

Second thought, I'll create a file with non-standard chunks to test it. Right now I'm away from my keyboard, I'll do it when I get back to home.

RonaldAJ commented 11 months ago

@iluvcapra Is it possible that I simply upload a big file as dummy, so that coverage test will pass even though the newly added code path wasn't reached?

Second thought, I'll create a file with non-standard chunks to test it. Right now I'm away from my keyboard, I'll do it when I get back to home.

I checked the large file storage options, but because of the bandwidth limitations it could get quite costly. So I think we can rule that one out.

Other option might be to generate the stream, that is simulate a large file. Not knowing the file format, I don't know how feasible that is. With a properly seeded random number generator you can make reproducible tests without storing a large amount of data.

dannyniu commented 11 months ago

I've written a POSIX shell script that generates a over-4GiB-chunks file. It'll be good if you can tell me how to incorporate it into the coverage test.

Alternatively, we can decide on some better compression algorithm (e.g. Bzip2, XZ, Zstd, etc.) to upload the file.

mknul-rf64.txt

iluvcapra commented 11 months ago

These files are too big to test it appears.

tests/test_adm.py .... [ 14%] tests/test_cue.py ..... [ 32%] tests/test_dolby.py ..... [ 50%] tests/test_main.py ... [ 60%] tests/test_rf64.py Error: The operation was canceled.

I appreciate the contribution but I'm kinda reticent to incorporate support for files that just don't appear in the wild and that we can't practically test within GitHub CI. There might be some other way of testing this change with mocks or virtualizing the test WAVE file, seems like a lot of work though.

dannyniu commented 11 months ago

@iluvcapra Fine. If anyone needs large chunks support, they can come to my fork then.

iluvcapra commented 11 months ago

I'll keep thinking about it, it should be possible to test for this with a mock file object it's just more work.

dannyniu commented 10 months ago

@iluvcapra In case you're wondering, the next POSIX / Single Unix Specification standard is to include support for "holes" in files - something I've been following, although, I'm not sure how much support it has on various platforms.