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

Fix end frame replacement to acknowledge the chunk size. #1320

Merged

Conversation

carlosfelgarcia
Copy link
Contributor

Link the Issue(s) this Pull Request is related to.
Fixes #1129

Summarize your change.
Simple change to replace the "FRAME_END" variable, acknowledging the chunk size by selecting the minimum between the last frame and last frame in the chunk.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 3, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@anton-ubi
Copy link

anton-ubi commented Oct 4, 2023

Hi there, thank you for doing this!

We met the same problem recently and this is nice to see this being addressed.

I wonder if the logic is correct though. Wouldn't it be the indexes we want to compare, rather than the frame numbers themselves ?

Let's take that example:

chunkSize = 3
frameRange = "1,2,3,4,99"  # Note the frame 99 at the end

With that configuration, I'd expect the frame set to be divided into two chunks:

[1, 2, 3] and [4, 99] where #FRAME_START# and #FRAME_END# are respectively 1 and 3 for chunk 1, and 4 and 99 for chunk 2.

Now with the current proposal, #FRAME_END# of the 2nd chunk is 6 (frameNumber + chunkSize - 1 being less than 99). That seems to be an incorrect behavior.


I'd suggest to follow the behavior of the getChunk method of the FrameSet java object, as introduced by @donalm here
The comparaison is made on the indexes rather than the values. We take the lesser one, and only then we convert to the corresponding frame value.

What do you think ?

@carlosfelgarcia
Copy link
Contributor Author

Yes, you are absolutely right, will revisit in the coming days.
Thank you!

@DiegoTavares
Copy link
Collaborator

Hi @carlosfelgarcia any update on this?

@carlosfelgarcia
Copy link
Contributor Author

Hi Diego, this has totally got out of my radar, I just updated the code.

@DiegoTavares
Copy link
Collaborator

Can you rebase this from master to make sure you get the fixes to failing unit tests merged in?

@carlosfelgarcia carlosfelgarcia force-pushed the fix-end-frame branch 2 times, most recently from fd7e7bf to edf899e Compare May 22, 2024 23:47
@DiegoTavares DiegoTavares merged commit 6c0991e into AcademySoftwareFoundation:master Jun 5, 2024
12 checks passed
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.

#FRAME_END# token wrong behaviour
3 participants