Rework wait for segment code#2076
Conversation
|
Decision: APPROVE Issues: none Evidence:
Review Details
|
CastagnaIT
force-pushed
the
wait_segments
branch
5 times, most recently
from
634c851 to
38dfda6
Compare
|
@kodiai review |
CastagnaIT
added
Type: Fix
kodiai responseI ran out of steps analyzing this and wasn't able to post a complete response. No review findings were published for this request. This usually means the agent got stuck in tool use or exceeded its step budget for this run. |
|
@kodiai review |
kodiai responseI ran out of steps analyzing this and wasn't able to post a complete response. No review findings were published for this request. This usually means the agent got stuck in tool use or exceeded its step budget for this run. |
CastagnaIT
force-pushed
the
wait_segments
branch
from
38dfda6 to
117f11e
Compare
|
@kodiai review |
Kodiai Review SummaryWhat ChangedThis PR refactors wait-for-segment buffering logic from per-representation to centralized AdaptiveStream management. Reviewed: core logic ObservationsImpact[CRITICAL] src/common/AdaptiveStream.cpp (1285): Null pointer dereference of thread_data_ [CRITICAL] src/common/AdaptiveStream.cpp (1281-1295): Null pointer dereferences of current_rep_ and current_period_ [MAJOR] src/common/AdaptiveStream.cpp (1283-1300): Race condition on m_isWaitingForSegment flag [MAJOR] src/common/SegmentBuffer.cpp (199): Off-by-one error in boundary validation [MAJOR] src/common/AdaptiveStream.cpp (976-977): Null pointer dereference in logging Verdict🔴 Address before merging -- 5 blocking issue(s) found |
When you are waiting for buffer, the VP do DemuxRead callbacks in very fast way causing CPU spikes until 13% (on a fast CPU) add a thread sleep help to mitigate the problem
CastagnaIT
force-pushed
the
wait_segments
branch
from
117f11e to
2281ec1
Compare
|
Only this was taken into account, the others are irrelevant on how the code works atm
the following one, should not happens because CSession::GetNextSample guard the call with IsReadSampleAsyncWorking
|
1 participant
Description
This will rework the WaitingForSegment code, moving it from
CRepresentationclass to theAdaptiveStreamclassthe
AdaptiveStreamclass will be the main place where manage buffering cases, without having to manage buffering per representation (stream) basis,IMO i don't see negative effects doing this change since one
AdaptiveStreamcan manage only one stream at timeMotivation and context
fix #2075
1° problem:
from PR #1867 has been added
InsertLiveSegmentcall insideAdaptiveStream::waitingForSegmentthis is good when called from
CSession::GetNextSamplebut this cause wrong things when called from demuxers
what happens, the demuxer try to read data from a segment, and if there is no segment
AdaptiveStream::ensureSegmentset IsWaitForSegment on the representation to "true", after this the demuxer callAdaptiveStream::waitingForSegmentto check if has to trigger EOS, but callingAdaptiveStream::waitingForSegmentcause to callInsertLiveSegmentthat invalidate the IsWaitForSegment status previously set, triggering EOS playback will freeze/stallso the demuxers must not cause a call to
InsertLiveSegment2° problem:
when there are no new segments
CSession::GetNextSamplecallAdaptiveStream::waitingForSegmentto callInsertLiveSegment, but ifInsertLiveSegmentat the moment dont insert new segments, cause a "cat and mice" behaviour like the following:between ensureSegment and InsertLiveSegment, so InsertLiveSegment should set IsWaitForSegment to false only when a new segment exists
3° problem:
When the stream is on buffering (e.g. loading wheel on GUI) the CPU starts to get very busy,
by debugging with in my PC (16 core, not so slow) i can observe CPU spike from 8% to 13% on Kodi when buffering (and fan speed go high), when playback back to normal CPU go down to about 1-7%,
i discovered that kodi VP do callbacks
DemuxReadin a uncontrolled loopinputstream.adaptive/src/main.cpp
Line 330 in c04fbd9
so when we are in buffering we return empty packets too fast, this cause the CPU spikes.
Add a thread sleep mitigate the problem, but seem that there is something else on VP to be fixed that cause slightly higher CPU usage
How has this been tested?
Screenshots (if appropriate):
Types of change
Checklist: