Chromecast: fix handling of streams with different durations

Chromecast: fix handling of streams with different durations

When one of the streams is much shorter than other(s), the
shorter stream reaches EOS first and then its media time
stops increasing, which blocks other streams due to using
BalancedMediaTaskRunner. Resetting the media task runner
will ensure other streams are not blocked by the shortest
one.

BUG=internal b/19560149

Committed: https://crrev.com/a2b2e005113c6b79690745ea4035244e14dfab98
Cr-Commit-Position: refs/heads/master@{#329557}

[servolk, 2015-05-13 01:15:01]

servolk@chromium.org changed reviewers:
+ damienv@chromium.org, gunsch@chromium.org, lcwu@chromium.org

[servolk, 2015-05-13 01:16:44]

On 2015/05/13 01:15:22, servolk wrote:

Note: I've tested this fix already. Started working on internal CL with updated
unit test for this case
(https://eureka-internal-review.git.corp.google.com/#/c/34373/), but haven't
finished the unit test work yet. Le-Chun asked me to merge just the fix ASAP to
have it included into 1.14. So I'll add unit test later.

[gunsch, 2015-05-13 01:18:46]

lgtm! I like this solution much better than anything else that's been proposed
so far.

[gunsch, 2015-05-13 01:19:02]

(but curious to see if Damien agrees)

[servolk, 2015-05-13 01:20:15]

The CQ bit was checked by servolk@chromium.org

[commit-bot: I haz the power, 2015-05-13 01:21:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1140003002/1

[commit-bot: I haz the power, 2015-05-13 01:30:22]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-05-13 01:31:09]

Patchset 1 (id:??) landed as
https://crrev.com/a2b2e005113c6b79690745ea4035244e14dfab98
Cr-Commit-Position: refs/heads/master@{#329557}

[damienv1, 2015-05-13 01:33:08]

Agree there might be an issue. Not sure this is the correct fix either (I'll
post some comments).

[damienv1, 2015-05-13 01:37:43]

https://codereview.chromium.org/1140003002/diff/1/chromecast/media/cma/filter...
File chromecast/media/cma/filters/demuxer_stream_adapter.cc (right):

https://codereview.chromium.org/1140003002/diff/1/chromecast/media/cma/filter...
chromecast/media/cma/filters/demuxer_stream_adapter.cc:190:
ResetMediaTaskRunner();
1) I don't think this fully address the issue:
e.g. let's assume you are in a state where video waits for audio to progress
(i.e. deltaTime between video and audio is greater than 3s: see
kMaxDeltaFetcher) then receiving the new buffer (which notifies the end of the
audio stream) is not going to trigger a resume on the video side.

2) + improvement: Instead of creating another task runner, only the previous one
should be destroyed (i.e. reset to NULL).

[damienv1, 2015-05-13 01:38:49]

+ out of curiosity: does this use case (audio stream finish 3s before video or
vice versa) happen in MSE or regular URL playback ?

[chromium-reviews, 2015-05-13 01:44:55]

Thanks for your comments Damien!
It's not clear to me at the moment, how we could get into state described
in #1 - the only case that comes to mind is if one stream is shorter than
the other, but after this fix, once we reach EOS of the shorter stream the
time diff shouldn't matter, since the task runner for ended stream will be
reset/removed from balanced task runner.
Re 2: I agree we could have just deleted the task runner, but I thought it
would be safer to just do Reset, since we already do that in Flush to
prepare for a new timeline, so this state is guaranteed to be handled
properly. So we are guaranteed not to crash if e.g. some code tried to
access media_task_runner_ after EOS is reached.

On Tue, May 12, 2015 at 6:38 PM, <damienv@chromium.org> wrote:

> + out of curiosity: does this use case (audio stream finish 3s before
> video or
> vice versa) happen in MSE or regular URL playback ?
>
>
> https://codereview.chromium.org/1140003002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2015-05-13 01:55:51]

It happened with mse. The internal bug has a link to repro stream, where
audio ends sooner than video. But I think regular .mp4 file could also
contain streams of diff length.
On May 12, 2015 6:38 PM, <damienv@chromium.org> wrote:

> + out of curiosity: does this use case (audio stream finish 3s before
> video or
> vice versa) happen in MSE or regular URL playback ?
>
>
> https://codereview.chromium.org/1140003002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[damienv1, 2015-05-13 01:57:32]

I agree this can happen only when one stream is shorter than the other.
However, all the timestamps used in the media task runner correspond to
timestamps that are ahead of the playback time (correspond to timestamps that
are being fetched).
So, you could possibly in a situation where:
- you fetch the last audio frame: e.g. PTS=2s
- you fetch the one video frame (let's say fetching of video is ahead): so e.g.
PTS=5.02s
  Here, the frame is not actually fetched since the difference is too big.
- then, the next audio frame is actually the EOS.
you destroy the corresponding task runner
and the video task runner is actually never woken up.

[servolk, 2015-05-13 19:02:13]

On 2015/05/13 01:57:32, damienv1 wrote:
> I agree this can happen only when one stream is shorter than the other.
> However, all the timestamps used in the media task runner correspond to
> timestamps that are ahead of the playback time (correspond to timestamps that
> are being fetched).
> So, you could possibly in a situation where:
> - you fetch the last audio frame: e.g. PTS=2s
> - you fetch the one video frame (let's say fetching of video is ahead): so
e.g.
> PTS=5.02s
>   Here, the frame is not actually fetched since the difference is too big.
> - then, the next audio frame is actually the EOS.
> you destroy the corresponding task runner
> and the video task runner is actually never woken up.

I think we could call BalancedMediaTaskRunnerFactory::OnNewTask from
BalancedMediaTaskRunnerFactory::UnregisterMediaTaskRunner after removing the
task runner. The OnNewTask will then calculate the new timestamp range without
the removed stream and will wake up any streams that can progress now. My only
concern - would that cause too much unnecessary work during seeking, when we
flush streams and reset task runners?

[chromium-reviews, 2015-05-13 19:06:39]

Here is what I meant:
https://codereview.chromium.org/1127183008

On Wed, May 13, 2015 at 12:02 PM, <servolk@chromium.org> wrote:

> On 2015/05/13 01:57:32, damienv1 wrote:
>
>> I agree this can happen only when one stream is shorter than the other.
>> However, all the timestamps used in the media task runner correspond to
>> timestamps that are ahead of the playback time (correspond to timestamps
>> that
>> are being fetched).
>> So, you could possibly in a situation where:
>> - you fetch the last audio frame: e.g. PTS=2s
>> - you fetch the one video frame (let's say fetching of video is ahead): so
>>
> e.g.
>
>> PTS=5.02s
>>    Here, the frame is not actually fetched since the difference is too
>> big.
>> - then, the next audio frame is actually the EOS.
>> you destroy the corresponding task runner
>> and the video task runner is actually never woken up.
>>
>
> I think we could call BalancedMediaTaskRunnerFactory::OnNewTask from
> BalancedMediaTaskRunnerFactory::UnregisterMediaTaskRunner after removing
> the
> task runner. The OnNewTask will then calculate the new timestamp range
> without
> the removed stream and will wake up any streams that can progress now. My
> only
> concern - would that cause too much unnecessary work during seeking, when
> we
> flush streams and reset task runners?
>
> https://codereview.chromium.org/1140003002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.