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 hanging playback when no stream is available #1562

Merged
6 changes: 3 additions & 3 deletions packages/app/app/actions/queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,16 @@ describe('Queue actions tests', () => {

describe('finds streams for track', () => {
const getSelectedStreamProvider = jest.spyOn(QueueOperations, 'getSelectedStreamProvider');
const getTrackStreams = jest.spyOn(QueueOperations, 'getTrackStreams');
const resolveTrackStreams = jest.spyOn(QueueOperations, 'resolveTrackStreams');

afterEach(() => {
getSelectedStreamProvider.mockReset();
getTrackStreams.mockReset();
resolveTrackStreams.mockReset();
});

test('remote track is removed from the queue when no streams are available', () => {
// Mock an empty search result for streams.
getTrackStreams.mockResolvedValueOnce([]);
resolveTrackStreams.mockResolvedValueOnce([]);

// Configure a dummy stream provider. It is not actually used in this execution path.
getSelectedStreamProvider.mockReturnValueOnce({});
Expand Down
73 changes: 58 additions & 15 deletions packages/app/app/actions/queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,8 @@
return _.find(streamProviders, { sourceName: selected.streamProviders });
};

export const getTrackStreams = async (
// Exported to facilitate testing.
export const resolveTrackStreams = async (
track: Track | LocalTrack,
selectedStreamProvider: StreamProvider
) => {
Expand Down Expand Up @@ -168,20 +169,18 @@

export const findStreamsForTrack = (index: number) => async (dispatch, getState) => {
const {queue, settings}: RootState = getState();

const track = queue.queueItems[index];

if (track && !track.local && isEmpty(track.streams)) {
dispatch(updateQueueItem({
...track,
loading: true
}));
if (track && !track.local && trackHasNoFirstStream(track)) {
if (!track.loading) {
dispatch(updateQueueItem({
...track,
loading: true
}));
}
const selectedStreamProvider = getSelectedStreamProvider(getState);
try {
let streamData = await getTrackStreams(
track,
selectedStreamProvider
);
let streamData: TrackStream[] = await getTrackStreams(track, selectedStreamProvider);

if (settings.useStreamVerification) {
try {
Expand All @@ -207,10 +206,14 @@
if (streamData?.length === 0) {
dispatch(removeFromQueue(index));
} else {
streamData = [
await selectedStreamProvider.getStreamForId(streamData.find(Boolean)?.id),
...streamData.slice(1)
];
streamData = await resolveSourceUrlForTheFirstStream(streamData, selectedStreamProvider);

const firstStream = streamData[0];
if (!firstStream?.stream) {
const remainingStreams = streamData.slice(1);
removeFirstStream(track, index, remainingStreams, dispatch);
return;
}

dispatch(
updateQueueItem({
Expand Down Expand Up @@ -240,6 +243,46 @@
}
};

async function getTrackStreams(track: QueueItem, streamProvider: StreamProvider): Promise<TrackStream[]> {
if (isEmpty(track.streams)) {
return resolveTrackStreams(
track,
streamProvider
);
}
return track.streams;
}

async function resolveSourceUrlForTheFirstStream(trackStreams: TrackStream[], streamProvider: StreamProvider): Promise<TrackStream[]> {
if (isEmpty(trackStreams[0].stream)) {
return [
await streamProvider.getStreamForId(trackStreams.find(Boolean)?.id),
...trackStreams.slice(1)
];
}
// The stream URL might already be resolved, for example for a previously played track.
return trackStreams;

Check warning on line 264 in packages/app/app/actions/queue.ts

View check run for this annotation

Codecov / codecov/patch

packages/app/app/actions/queue.ts#L264

Added line #L264 was not covered by tests
}

export function trackHasNoFirstStream(track: QueueItem): boolean {
return isEmpty(track?.streams) || isEmpty(track.streams[0].stream);
}

function removeFirstStream(track: QueueItem, trackIndex: number, remainingStreams: TrackStream[], dispatch): void {
if (remainingStreams.length === 0) {
// no more streams are available
dispatch(removeFromQueue(trackIndex));
} else {
// remove the first (unavailable) stream
dispatch(updateQueueItem({
...track,
loading: true,
error: false,
streams: remainingStreams
}));
}
}

export function playTrack(streamProviders, item: QueueItem) {
return (dispatch) => {
dispatch(clearQueue());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ describe('Album view container', () => {
artist: 'test artist',
name: 'test track 1',
streams: [{
data: 'test-stream-data',
stream: 'test-stream-url',
author: {
name: 'test author'
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,8 @@ describe('PlayerBar container', () => {
streams: [{
duration: 300,
title: 'test track 1',
skipSegments: []
skipSegments: [],
stream: 'stream URL'
}]
}]
},
Expand All @@ -154,15 +155,17 @@ describe('PlayerBar container', () => {
streams: [{
duration: 300,
title: 'test track 1',
skipSegments: []
skipSegments: [],
stream: 'stream URL 1'
}]
}, {
artist: 'test artist 2',
name: 'test track 2',
streams: [{
duration: 300,
title: 'test track 2',
skipSegments: []
skipSegments: [],
stream: 'stream URL 2'
}]
}]
},
Expand All @@ -187,7 +190,8 @@ describe('PlayerBar container', () => {
streams: [{
duration: 300,
title: 'test track 1',
skipSegments: []
skipSegments: [],
stream: 'stream URL 1'
}]
}, {
artist: 'test artist 2',
Expand All @@ -200,7 +204,8 @@ describe('PlayerBar container', () => {
duration: 10,
endTime: 10,
startTime: 0
}]
}],
stream: 'stream URL 2'
}]
}]
},
Expand All @@ -225,7 +230,8 @@ describe('PlayerBar container', () => {
streams: [{
duration: 300,
title: 'test track 1',
skipSegments: []
skipSegments: [],
stream: 'stream URL'
}]
}]
},
Expand Down Expand Up @@ -272,6 +278,53 @@ describe('PlayerBar container', () => {
});
});

it('should remove the track when no more stream URLs can be resolved', async () => {
const { component, store } = mountComponent({
queue: {
currentSong: 0,
queueItems: [
{
uuid: 'uuid1',
artist: 'test artist name',
name: 'track without streams'
}
]
},
plugin: {
plugins: {
streamProviders: [
{
sourceName: 'Mocked Stream Provider',
search: jest.fn().mockResolvedValueOnce([
{
id: 'stream 1 ID',
source: 'Mocked Stream Provider'
},
{
id: 'stream 2 ID',
source: 'Mocked Stream Provider'
}
]),
getStreamForId: jest.fn()
.mockResolvedValueOnce(null)
.mockResolvedValueOnce({
stream: null,
source: 'Mocked Stream Provider'
})
}
]
},
selected: {
streamProviders: 'Mocked Stream Provider'
}
}
});
await waitFor(() => {
const state = store.getState();
expect(state.queue.queueItems.length).toBe(0);
});
});

const mountComponent = (initialStore?: AnyProps) => {
const store = configureMockStore({
...buildStoreState()
Expand Down
27 changes: 20 additions & 7 deletions packages/app/app/containers/PlayerBarContainer/hooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,11 @@ import * as playerActions from '../../actions/player';
import * as queueActions from '../../actions/queue';
import * as settingsActions from '../../actions/settings';
import * as favoritesActions from '../../actions/favorites';
import { favoritesSelectors } from '../../selectors/favorites';
import { favoritesSelectors, getFavoriteTrack } from '../../selectors/favorites';
import { playerSelectors } from '../../selectors/player';
import { queue as queueSelector } from '../../selectors/queue';
import { settingsSelector } from '../../selectors/settings';
import { getFavoriteTrack } from '../../selectors/favorites';
import { QueueItem } from '../../reducers/queue';
import { QueueItem, QueueStore } from '../../reducers/queue';

export const useSeekbarProps = () => {
const dispatch = useDispatch();
Expand Down Expand Up @@ -255,12 +254,10 @@ export const useStreamLookup = () => {
const queue = useSelector(queueSelector);

useEffect(() => {
const isStreamLoading = Boolean(queue.queueItems.find((item) => item.loading));

if (!isStreamLoading) {
if (shouldSearchForStreams(queue)) {
const currentSong: QueueItem = queue.queueItems[queue.currentSong];

if (currentSong && isEmpty(currentSong.streams)) {
if (currentSong && queueActions.trackHasNoFirstStream(currentSong)) {
dispatch(queueActions.findStreamsForTrack(queue.currentSong));
return;
}
Expand All @@ -273,3 +270,19 @@ export const useStreamLookup = () => {
}
}, [queue]);
};

const shouldSearchForStreams = (queue: QueueStore): boolean => {
const currentlyLoadingTrack = queue.queueItems.find((item) => item.loading);
if (!currentlyLoadingTrack) {
// No track is currently "loading": start searching for steams.
return true;
}
if (isEmpty(currentlyLoadingTrack.streams)) {
// Streams are not yet resolved: this happens when the track is being marked as "loading"
// while still resolving streams. We don't need to start a search until that operation completes or fails.
return false;
}
const firstStream = currentlyLoadingTrack.streams[0];
// A search should be performed if the first stream URL wasn't yet resolved.
return !firstStream?.stream;
};
6 changes: 2 additions & 4 deletions packages/app/test/storeBuilders.ts
Original file line number Diff line number Diff line change
Expand Up @@ -279,13 +279,12 @@ export const buildStoreState = () => {
sourceName: 'Test Stream Provider',
search: jest.fn(() => ([{
id: 'test-stream-id',
data: 'test-stream-data',
author: {
name: 'test author'
}
}])),
getStreamForId: jest.fn(() => ({
data: 'test-stream-data',
stream: 'test-stream-url',
author: {
name: 'test author'
}
Expand All @@ -296,15 +295,14 @@ export const buildStoreState = () => {
sourceName: 'Different Stream Provider',
search: jest.fn(() => ([{
id: 'test-stream-id-1',
data: 'different-test-stream-data',
source: 'Different Stream Provider',
author: {
name: 'test author'
}
}])),
getStreamForId: jest.fn((id: string) => ({
id: 'test-stream-id-1',
data: id,
stream: 'different-test-stream-url',
source: 'Different Stream Provider'
}) as TrackStream)
}
Expand Down
Loading