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

[XB1] Fix skipped OnDecoderDrained task. #1765

Merged
merged 2 commits into from
Nov 6, 2023

Conversation

victorpasoshnikov
Copy link
Collaborator

b/284359403

@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Merging #1765 (c14f287) into main (36ddc12) will decrease coverage by 0.02%.
Report is 1 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1765      +/-   ##
==========================================
- Coverage   57.77%   57.76%   -0.02%     
==========================================
  Files        1915     1915              
  Lines       95119    95119              
==========================================
- Hits        54959    54947      -12     
- Misses      40160    40172      +12     

see 4 files with indirect coverage changes

@victorpasoshnikov
Copy link
Collaborator Author

The following has place in sw (vpx, av1) decoders.
In GpuVideoDecoderBase::Reset() we have call consequence

  1. decoder_thread_->job_queue()->Schedule(
    std::bind(&GpuVideoDecoderBase::DrainDecoder, this)); <- put task DrainDecoder into thread's job queue
  2. decoder_thread_.reset(); <- Put task JobQueue::StopSoon and join thread
    But DrainDecoder causes OnDecoderDrained() in different thread (underlaying decoder's output thread) and the task OnDecoderDrained() can be created AFTER JobQueue::StopSoon task and can't be processed.
    To avoid this situation I had to add extra sync objects to make sure that decoder_thread_.reset() will be called after OnDecoderDrained() call

Copy link
Contributor

@TyHolc TyHolc left a comment

Choose a reason for hiding this comment

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

Looks good to me, I'll leave it to the media team to give a final review.

@victorpasoshnikov
Copy link
Collaborator Author

This PR must be merged together with https://lbshell-internal-review.googlesource.com/c/cobalt_src/+/267860

b/284359403

Change-Id: I606ce22d1a48568196e0f01900491f6e86499336
b/284359403

Change-Id: I55c41775917c9bd96ceae28b53343a133cd9d3f0
@TyHolc TyHolc merged commit 8eea4c9 into youtube:main Nov 6, 2023
333 of 334 checks passed
@TyHolc TyHolc added the cp-24.lts.1+ Cherry Pick to the 24.lts.1+ branch label Nov 6, 2023
cobalt-github-releaser-bot pushed a commit that referenced this pull request Nov 6, 2023
b/284359403

(cherry picked from commit 8eea4c9)
gbournou pushed a commit that referenced this pull request Nov 6, 2023
Refer to the original PR: #1765

b/284359403

Co-authored-by: victorpasoshnikov <[email protected]>
Rongo-JL pushed a commit to Rongo-JL/cobalt that referenced this pull request Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cp-24.lts.1+ Cherry Pick to the 24.lts.1+ branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants