-
Notifications
You must be signed in to change notification settings - Fork 1k
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
API to enable slideshow keyframes below a spatial layer #1297
API to enable slideshow keyframes below a spatial layer #1297
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I just have some minor questions
ELOG_DEBUG("message: Setting slideshow fallback, below_min_layer %u, spatial_layer %d," | ||
"next_spatial_layer %d slidehow_fallback_active_: %d", | ||
below_min_layer, spatial_layer_, next_spatial_layer, slideshow_fallback_active_); | ||
"next_spatial_layer %d slidehow_fallback_active_: %d, min_desired_spatial_layer: %d", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be called min_requested_spatial
like you do above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, actually I think I want both min_requested_spatial_layer
and slideshow_below_spatial_layer_
to debug possible problems
"next_spatial_layer %d slidehow_fallback_active_: %d", | ||
below_min_layer, spatial_layer_, next_spatial_layer, slideshow_fallback_active_); | ||
"next_spatial_layer %d slidehow_fallback_active_: %d, min_desired_spatial_layer: %d", | ||
below_min_layer, spatial_layer_, next_spatial_layer, freeze_fallback_active_, slideshow_below_spatial_layer_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above should we use min_requested_spatial_layer
here?
void QualityManager::enableSlideShowBelowSpatialLayer(int spatial_layer) { | ||
ELOG_DEBUG("message: Will activate slideshow when below spatial layer, spatial_layer: %d", spatial_layer); | ||
slideshow_below_spatial_layer_ = spatial_layer; | ||
freeze_fallback_active_ = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a way to disable this feature afterwards?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed this in the new commit 👍
I've changed the API to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
aux_spatial_layer, min_valid_spatial_layer); | ||
} | ||
aux_temporal_layer = 0; | ||
aux_spatial_layer++; | ||
} | ||
|
||
if (below_min_layer != slideshow_fallback_active_) { | ||
ELOG_DEBUG("below_min_layer %u, freeze_fallback_active_: %u", below_min_layer, freeze_fallback_active_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nick: I know we don't follow a strict patter for DEBUG messages, but all the other messages in this class start with message:
would be very nice to also have an API to force the minimum temporal layer, since, especially on lower spatial layer, since in terms of kbytes the difference is really small. I think that in the user experience, one may prefer to see a fluid smaller spatial layer instead of one laggy (ex. spatial 1 and temporal 0) but with better resolution. So the quality manager will switch only on preferred ones |
Description
This PR updates #1199, adding the possibility to enable SlideShow when below layer 0, effectively replacing Fallback mode if desired.
This is useful, for example, when activating simulcast with Screen sharing. Instead of getting a frozen frame, users with low bandwidth will get an updated image (of the lowest possible quality) every few seconds.
Additionally, this PR updates the API to better reflect this behaviour and cleans up some of the variable names.
[] It needs and includes Unit Tests
Changes in Client or Server public APIs
[] It includes documentation for these changes in
/doc
.