Fix processing FrameData with no frames #798
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
FedyuninV
commented
May 23, 2024
| @@ -95,7 +95,7 @@ void View::DrawTimelineFrames( const FrameData& frames ) | |||
| { | |||
| const std::pair <int, int> zrange = m_worker.GetFrameRange( frames, m_vd.zvStart, m_vd.zvEnd ); | |||
| if( zrange.first < 0 ) return; | |||
| if( m_worker.GetFrameBegin( frames, zrange.first ) > m_vd.zvEnd || m_worker.GetFrameEnd( frames, zrange.second ) < m_vd.zvStart ) return; | |||
There was a problem hiding this comment.
Here's additional moment: after checking for frames.frames.empty() I've faced crash when GetFrameRange returned the frames.frames.end() as a zrange.second and m_worker.GetFrameEnd( frames, zrange.second ) went OOB
I'm unable to check the full-scale app right now, but my small sample still fails (not every launch) and now in another place Could the fact that now you don't immediately call P.S. what about the line I commented before ( |
This can be ignored. (Not really, but a fix is non-trivial and you get the warning in the UI that data is invalid until a save+load cycle.)
Can you provide output of an asan-enabled build that shows this is a problem? |
Agreed, I've reproduced the same problem on my branch too, so it's probably unrelated with the problem in hand.
Got it on a very same sample That was the problem: if frame is not continuous, |
|
Should be fixed by d3da7dc. |
Small example seems stable. I won't be able to test it on full app till Monday. |
|
Your fix worked smoothly. Closing both PR and issure. Thanks a lot! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm trying to solve the issue #789
The #666 partially fixed the problem by allowing us to not use
DeferItemat all. But it causes some stability issues: ignoring of unpairedMarkEndleavesFrameDataobject with emptyframes. But the server processing is built around the fact, thatFrameData::framesare guaranteed to be filled with something (e.g.Worker::GetFrameRangegoes out-of-bounds ifFrameData::framesis empty). We've faced this while doing the following:TRACY_ON_DEMANDtracy-profilerand leave it waiting for127.0.0.1tracy-profilercrashesThis PR tries to close all possibly unsafe places (
GetFrameBegin/End/Rangeshouldn't be called whenFrameData::framesis empty). The usual stacktrace locally isView::DrawTimelineFrames( *fd )->Worker::GetFrameRange( *fd )->out-of-bounds on zitbegin--The small synthetic example which crashes the master but seems stable with my fix. It should be placed in
examples/test_delay.CMakeLists.txt
main.cpp
P.S. I thought about a way to not let
FrameDatalike these into processing in the first place (filtering them somehow) but didn't find a proper place in the code.