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.

Sign up for GitHub

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

Jump to bottom

Some cleanup to AdaptiveStream and Session #1351

Merged
merged 11 commits into from
Aug 15, 2023
Merged

Some cleanup to AdaptiveStream and Session #1351

merged 11 commits into from
Aug 15, 2023

Conversation

CastagnaIT
Copy link
Collaborator

@CastagnaIT CastagnaIT commented Aug 5, 2023 •
edited
Loading

Description

Some cleanup to AdaptiveStream and Session should not functionality change some details on commits
the only exception is that the Choosers now can get callbacks for stream different than VIDEO (audio/subs) that however are already handled

Motivation and context

i was investigating to improve HLS parser and im stuck on some of these to do future cleanups

How has this been tested?

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • I have read the Contributing document
  • My code follows the Code Guidelines of this project
  • My change requires a change to the Wiki documentation
  • I have updated the documentation accordingly

@CastagnaIT
[AdaptiveStream] Removed choose_rep_
07c6ef9 
The CSession::AddStream already provide the initial representation based on the same chooser GetRepresentation() method
@CastagnaIT
[AdaptiveStream] removed prepareRepresentation
41cbb59 
This is already done in the CInputStreamAdaptive::OpenStream by
m_session->PrepareStream method
@CastagnaIT
[AdaptiveStream] Removed unused rep_counter_ prev_rep_
f1af5c6
@CastagnaIT
[AdaptiveStream] Add descriptions to buffer variables
3c7bc88
@CastagnaIT CastagnaIT added Type: Cleanup non-breaking change which removes non-working or unmaintained functionality v21 Omega labels Aug 5, 2023
@CastagnaIT CastagnaIT added the WIP label Aug 5, 2023
@CastagnaIT
Copy link
Collaborator Author

i dont know reason but dash dont works anymore, i will need to investigate

@CastagnaIT CastagnaIT removed the WIP label Aug 5, 2023
@CastagnaIT
Copy link
Collaborator Author

problem fixed
was because i had removed the call to InitializeDRM from CSession::InitializePeriod for unencrypted streams case,
i thought it would be more intuitive to call InitializeDRM only when you have encrypted content, but do this requires more cleanup to the code than i thought so i revert the change

glennguy
glennguy approved these changes Aug 13, 2023
View reviewed changes
src/common/AdaptiveStream.cpp Outdated
Comment on lines 856 to 861
// Dont change representation if it is the last segment of a period
if (isLastSegment)
newRep = prevRep;
Copy link
Contributor

Choose a reason for hiding this comment

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

I forgot about this.

I don't think my existing code was correct because I noticed a crash on this once. It was a long time ago so I can't remember the exact details - hoping that your change from using valid_segment_buffers_ to available_segment_buffers_ is where my error was.

FYI the reason behind adding this condition was that when it comes time to play the last segment in a period, AdaptiveStream wasn't able to insert the initialization segment (in the case of fMP4) and you would get corrupted or blank video for the last segment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm better if i add this as comment in the code

@CastagnaIT
[AdaptiveStream] Simplified conditions for representations
eda9f05 
This also remove the forced limitation to allow choosers for video stream only
@CastagnaIT
[Session] Fix possible timingStream nullptr access
05f0877 
if an error occours CSession::EnableStream erase the timingStream pointer but
CInputStreamAdaptive::DemuxRead continue to receive callbacks from kodi
that can cause a crash
@CastagnaIT
[Chooser] Moved PostInit to end of session init
1e607ec 
Avoid that PostInit to be called more times during playback
@CastagnaIT
[Session] Use c++ init for m_cdmSessions objects
d87ee44
@CastagnaIT
[Session] Avoid print "reusing DRM psshSets" when not so
726ae0d 
This was printed also playing not encrypted content
@CastagnaIT
[ChooserDefault] Limit resolution check from video stream
343d864
@CastagnaIT
[ChooserDefault] Removed a log condition
43c1966
@CastagnaIT CastagnaIT merged commit 5ffa8fb into xbmc:Omega Aug 15, 2023
9 checks passed
@CastagnaIT CastagnaIT deleted the cleanups branch August 15, 2023 05:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glennguy glennguy glennguy approved these changes

Assignees
No one assigned
Labels
Type: Cleanup non-breaking change which removes non-working or unmaintained functionality v21 Omega
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@CastagnaIT @glennguy