Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow up to 255 sequences to be scanned. #382

Merged
merged 1 commit into from Jun 20, 2021

Conversation

AliceLR
Copy link
Contributor

@AliceLR AliceLR commented Jun 19, 2021

The sequence_control array theoretically allows up to 255 sequences to exist, but because player_data::scan and module_data::seq_data are allocated as part of their respective structs, the sequence count is limited to 16. This patch raises that limit and moves player_data::scan, the larger of the two scan arrays, to a separate heap allocation.

Large numbers of sequences may be used in games relying on dynamic order changes, which is somewhat common in MegaZeux games (the module I was given, which I'm not sure I'm allowed to share, contains 18). Since the scan doubles as a validity check for module flow, increasing the number of sequences is the easiest way to safely support this.

This should not affect the API at all: xmp_module_info already uses a pointer to an array of an arbitrary number of xmp_sequence structs, and since this array is still allocated as part of the xmp_context, this pointer will never change during the lifetime of any given xmp_context. player_data::scan was never exposed to the API, so (re)allocating it should break nothing.

@codecov
Copy link

codecov bot commented Jun 19, 2021

Codecov Report

Merging #382 (69325b4) into master (0122a73) will increase coverage by 0%.
The diff coverage is 76%.

❗ Current head 69325b4 differs from pull request most recent head a5c7387. Consider uploading reports for the commit a5c7387 to get more accurate results
Impacted file tree graph

@@          Coverage Diff           @@
##           master    #382   +/-   ##
======================================
  Coverage      83%     83%           
======================================
  Files         149     149           
  Lines       27588   27603   +15     
======================================
+ Hits        22956   22970   +14     
- Misses       4632    4633    +1     
Impacted Files Coverage Δ
src/common.h 100% <ø> (ø)
src/dataio.c 97% <ø> (ø)
src/depackers/depacker.c 77% <ø> (ø)
src/depackers/gunzip.c 79% <ø> (ø)
src/depackers/muse.c 90% <ø> (ø)
src/depackers/oxm.c 81% <ø> (ø)
src/depackers/unxz.c 84% <ø> (ø)
src/depackers/xz_config.h 100% <ø> (ø)
src/effects.c 83% <ø> (ø)
src/extras.c 94% <ø> (ø)
... and 76 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9a07ee...a5c7387. Read the comment docs.

@AliceLR
Copy link
Contributor Author

AliceLR commented Jun 19, 2021

Manual AddressSanitizer (Fedora 34, clang 12) and Visual Studio (x86_64, MSVC 2019) tests passed. (It seems that at some point Fedora and/or clang's MemorySanitizer support for stat() without instrumentation broke, so I don't have a useful result there. The tests get spammed with failures due to struct stat st being "uninitialized" in load.c.)

Copy link
Collaborator

@sezero sezero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fair to me. @cmatsuoka, @sagamusix: Is this OK?

@sagamusix
Copy link
Contributor

Concept-wise it's all fine by me, can't comment much on the implementation itself

@sezero
Copy link
Collaborator

sezero commented Jun 20, 2021

Mergin this. Let's see how it goes.

@sezero sezero merged commit 3e1e556 into libxmp:master Jun 20, 2021
@AliceLR AliceLR deleted the allow-extra-sequences branch June 20, 2021 00:15
@AliceLR AliceLR added this to the 4.5.1 milestone Jul 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants