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 seqno jumps when bwe is close to the lowest SVC layer #1070

Open
wants to merge 1 commit into
base: v3
Choose a base branch
from

Conversation

vpalmisano
Copy link
Contributor

It seems solving the issue described in #1069

patched
patch.pcapng.gz

@jmillan
Copy link
Member

jmillan commented Apr 23, 2023

I need to check it in detail but at a first glance I don't see how the issue is produced or how the fix solves it. Do you have an explanation for it?

@jmillan
Copy link
Member

jmillan commented Apr 23, 2023

It makes sense to sync when no layer can be sent 👍

@jmillan jmillan self-requested a review April 23, 2023 15:55
@vpalmisano
Copy link
Contributor Author

Do you have an explanation for it?

Probably the sequence numbers keep increasing even if no layers are sent.

@ggarber
Copy link
Contributor

ggarber commented Apr 24, 2023

@jmillan Isn't syncRequired marking when we are waiting for a keyframe (or something similar)? If layers are disabled is not clear for me why it should be syncRequired.

I mean, it looks clear that there is an (important) issue but I don't understand if changing syncRequired is fixing the issue or workarounding it.

Thx

@jmillan
Copy link
Member

jmillan commented Apr 24, 2023

Yes, let's get to the root cause here.

@ibc
Copy link
Member

ibc commented Apr 24, 2023

We are drinking in an offsite. Sorry for the delay.

@buptlsp
Copy link

buptlsp commented May 10, 2023

I also found a strange seqno issue. sqlno will jump per 20 packets in some scenarios. Currently I don't know what scenario. Can you please tell me if this pr of yours can solve this problem?

image
image

@buptlsp
Copy link

buptlsp commented May 12, 2023

@ibc can you reply me about this problem? this means in some situation, the packet will lost 5% percent.

@ibc
Copy link
Member

ibc commented May 12, 2023

@buptlsp, this is a PR with a possible fix for SvcConsumer (AKA VP9 codec with SVC). Please let's focus on the topic of the PR.

@buptlsp
Copy link

buptlsp commented May 12, 2023

the result is similar, the seq number will jumps. I don't know if it's a same issue. if this is only about svc, I will try to find the reason. thanks!

@ggarber
Copy link
Contributor

ggarber commented Jul 12, 2023

Should the "this->syncRequired = true;" be done when the layers are reenabled (layer: -1 -> 0) instead of when the layers are disabled (layer: 0 -> -1)?

@ibc
Copy link
Member

ibc commented Jul 12, 2023

We'll jump into this after some very busy days at work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants