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 init value for storeLastSettings #4221

Conversation

minhui-foxtel
Copy link
Contributor

@minhui-foxtel minhui-foxtel commented Jul 10, 2023

Hi here,
If streaming.lastMediaSettingsCachingInfo.enabled is disabled, from my understanding, dash.js should not take last selected track into account.
But from current implementation, if a a track set by MediaPlayer.setCurrentTrack(), it will be treated as lastSelectedTrack, see L209

lastSelectedTracks[type] = settings;

then it will affect which track will be selected, see L64
function setInitialMediaSettingsForType(type, streamInfo) {
let settings = lastSelectedTracks[type] || getInitialSettings(type);
const tracksForType = getTracksFor(type, streamInfo.id);

So need to get a way to avoid to set lastSelectedTrack when streaming.lastMediaSettingsCachingInfo is disabled. storeLastSettings seems to be a proper flag, see L194, but storeLastSettings of a track is hardcoded to true and never changed. This PR is to respect streaming.lastMediaSettingsCachingInfo.enabled and give a reasonable initial value.
if (!noSettingsSave) {
let settings = extractSettings(track);
if (!settings || !tracks[id][type].storeLastSettings) return;

@minhui-foxtel minhui-foxtel changed the title Fix/init value for store last settings Fix init value for storeLastSettings Jul 10, 2023
@@ -89,7 +89,7 @@ function MediaController() {
setTrack(selectInitialTrack(type, tracksForType), true);
} else {
if (tracks.length > 1) {
setTrack(selectInitialTrack(type, tracks, !!lastSelectedTracks[type]));
setTrack(selectInitialTrack(type, tracks));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

selectInitialTrack has only 2 arguments, maybe it's for other purposes, but it doesn't take any effects at the moment.

@dsilhavy dsilhavy added this to the 4.7.2 milestone Jul 12, 2023
@dsilhavy dsilhavy self-assigned this Jul 12, 2023
@dsilhavy
Copy link
Collaborator

@minhui-foxtel i think the idea behind lastMediaSettingsCachingInfo.enabled is to enable/disable the usage of settings that are stored in the local storage.

What is the exact order of calls that causes a problem for you here? attachSource should reset lastSelectedTracks. Are you using a multiperiod MPD?

@minhui-foxtel
Copy link
Contributor Author

@dsilhavy yep, I'm using multi period MPD, so no proper reason to call attachSource for the case.
Let me explain why I'd like to do this. I'm facing an incompatible audio codec issue when switch period. Let's see, there are two periods,

[
    { // Period 1
        audioTracks: [
            {
                codec: 'ec-3',
                audioChannelConfiguration: [2],
            },
            {
                codec: 'mp4a.40.2',
                audioChannelConfiguration: [2],
            },
        ],
    },
    { // Period 2
        audioTracks: [
            {
                codec: 'ec-3',
                audioChannelConfiguration: ['F801'],
            },
            {
                codec: 'mp4a.40.2',
                audioChannelConfiguration: [2],
            },
        ],
    },
];

If set audio track with ec-3 in Period 1, dash.js will use a different codec of track mp4a.40.2 for Period 2 since its audioChannelConfiguration [2] are the same as last selected track. see L85

if (settings) {
tracks = Array.from(tracksForType);
tracks = filterTracksBySettings(tracks, matchSettingsLang, settings);
tracks = filterTracksBySettings(tracks, matchSettingsIndex, settings);
tracks = filterTracksBySettings(tracks, matchSettingsViewPoint, settings);
if (!(type === Constants.AUDIO && !!lastSelectedTracks[type])) {
tracks = filterTracksBySettings(tracks, matchSettingsRole, settings);
}
tracks = filterTracksBySettings(tracks, matchSettingsAccessibility, settings);
tracks = filterTracksBySettings(tracks, matchSettingsAudioChannelConfig, settings);
}

But it will cause playback stall on some CTV devices, e.g. Samsung. It's probably due to incompatible codec, but I'm not very sure so far, I've checked if use the track with the same codec, playback will go well.
So I tend to select initial track by myself, setCustomInitialTrackSelectionFunction seems to be a good choice, in order to let dash.js hit the custom function, I have to avoid dash.js to get the value form lastSelectedTracks[type] || getInitialSettings(type). see
function setInitialMediaSettingsForType(type, streamInfo) {
let settings = lastSelectedTracks[type] || getInitialSettings(type);
const tracksForType = getTracksFor(type, streamInfo.id);

To achieve the goal, I disabled lastMediaSettingsCachingInfo.enabled and avoided to call mediaPlayer.setInitialMediaSettingsFor(), and it did work well. but if I select a track manually by mediaPlayer.setCurrentTrack(newTrack), the newTrack will be treated as last selected track, CustomInitialTrackSelectionFunction will not be called anymore. Adding an extra argument noSettingsSave to setCurrentTrack to prevent from saving the track as last selected track could also work around my issue.

function setCurrentTrack(track, noSettingsSave) {
    if (!streamingInitialized) {
        throw STREAMING_NOT_INITIALIZED_ERROR;
    }
    mediaController.setTrack(track, noSettingsSave);
}

But I think dash.js should have a way to prevent from setting last selected track.

Here's my code snippet,

const player = dashjs.MediaPlayer().create();
player.initialize(videoElement)
player.attachSource(source, startTime);

player.setCustomInitialTrackSelectionFunction(
    myTrackSelectionFunction
);

player.updateSettings({
    streaming: {
        lastMediaSettingsCachingInfo: {
            // dash.js takes `audioChannelConfiguration` into account but doesn't consider codec, 
            // so disable this and use `setCustomInitialTrackSelectionFunction` instead.
            enabled: false,
        },
    }
});

// Manually trigger this during Period 1, the newTrack will be set to last selected track, I need a way to prevent it.
player.setCurrentTrack(newTrack);

@dsilhavy
Copy link
Collaborator

Thanks for the detailed explanation. I would like to separate the logic for getting media settings from the local storage and saving the settings for a single streaming session.

  1. Let's add the noSettingsSave flag to Mediaplayer.setCurrentTrack as in your example above.
  2. If required, I suggest we introduce an additional settings flag like saveLastMediaSettingsForCurrentStreamingSession that enables/disables storeLastSettings as in your original PR

Does that work for you?

@minhui-foxtel
Copy link
Contributor Author

@dsilhavy Thanks for your help, can you please review the PR again? Here's updates:

  1. Added noSettingsSave flag to Mediaplayer.setCurrentTrack
  2. I still think a settings flag to enable/disable storeLastSettings is needed since there are still other ways to set lastSelectedTracks, e.g. select a track by initial settings or settings from local storage. So added this flag in the PR as well.

@dsilhavy
Copy link
Collaborator

@dsilhavy Thanks for your help, can you please review the PR again? Here's updates:

  1. Added noSettingsSave flag to Mediaplayer.setCurrentTrack
  2. I still think a settings flag to enable/disable storeLastSettings is needed since there are still other ways to set lastSelectedTracks, e.g. select a track by initial settings or settings from local storage. So added this flag in the PR as well.

Thanks your PR looks good to me. Minor comments: Please add the following to index.d.ts

  • the new setting saveLastMediaSettingsForCurrentStreamingSession
  • the missing noSettingsSave to setCurrentTrack

@minhui-foxtel
Copy link
Contributor Author

Updated index.d.ts @dsilhavy

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

Successfully merging this pull request may close these issues.

2 participants