1415766 - We never pass anything other than NS_SEEK_SET to MediaCacheStream::Seek()

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement, P3)

Description

[JW Wang [:jwwang] [:jw_wang]]

http://searchfox.org/mozilla-central/rev/c99d035f00dd894feff38e4ad28a73fb679c63a6/dom/media/MediaCache.cpp#2424-2439

We don't need the switch case at all.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1415766. P1 - we never pass anything other than NS_SEEK_SET to Seek().

Review commit: https://reviewboard.mozilla.org/r/197926/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/197926/

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1415766. P2 - move Seek() to private and tighten up some assertions.

Review commit: https://reviewboard.mozilla.org/r/197928/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/197928/

Comment 3

[Benjamin Chen [:bechen]]

Comment on attachment 8926684 [details]
Bug 1415766. P1 - we never pass anything other than NS_SEEK_SET to Seek().

https://reviewboard.mozilla.org/r/197926/#review203114

::: dom/media/MediaCache.cpp:31
(Diff revision 1)
>  #include "nsIPrincipal.h"
> -#include "nsISeekableStream.h"
>  #include "nsPrintfCString.h"
>  #include "nsProxyRelease.h"
>  #include "nsThreadUtils.h"
>  #include "prio.h"

Could we also remove the prio.h?

Comment 4

[Benjamin Chen [:bechen]]

Comment on attachment 8926685 [details]
Bug 1415766. P2 - move Seek() to private and tighten up some assertions.

https://reviewboard.mozilla.org/r/197928/#review203118

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8926684 [details]
Bug 1415766. P1 - we never pass anything other than NS_SEEK_SET to Seek().

https://reviewboard.mozilla.org/r/197926/#review203114

> Could we also remove the prio.h?

I will do that in a new bug.

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8926684 [details]
Bug 1415766. P1 - we never pass anything other than NS_SEEK_SET to Seek().

https://reviewboard.mozilla.org/r/197926/#review203126

LGTM.
Note that this is similar to bug 1374173, which was backed out for intermittent failures! But it was probably because of the other part of that bug, so I think this one should be fine; but please keep an eye on build results.

::: dom/media/MediaCache.cpp:2425
(Diff revision 1)
> -  default:
> -    NS_ERROR("Unknown whence");
>      return NS_ERROR_FAILURE;
>    }
> -
> +  int64_t oldOffset = mStreamOffset;
> +  int64_t newOffset = aOffset;

You don't really need `newOffset` anymore, you could just use `aOffset` throughout.

::: dom/media/MediaCache.cpp:2426
(Diff revision 1)
>    if (!IsOffsetAllowed(newOffset)) {
>      return NS_ERROR_FAILURE;
>    }

Minor optimization: Move this test&return before taking the monitor.

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8926685 [details]
Bug 1415766. P2 - move Seek() to private and tighten up some assertions.

https://reviewboard.mozilla.org/r/197928/#review203132

::: dom/media/MediaCache.cpp:2421
(Diff revision 1)
>  MediaCacheStream::Seek(int64_t aOffset)
>  {
> -  NS_ASSERTION(!NS_IsMainThread(), "Don't call on main thread");
> +  MOZ_ASSERT(!NS_IsMainThread());
> +  mMediaCache->GetReentrantMonitor().AssertCurrentThreadIn();
>  
> -  ReentrantMonitorAutoEnter mon(mMediaCache->GetReentrantMonitor());
> +  if (mClosed || !IsOffsetAllowed(aOffset)) {

As per my comment in the previous patch, the `IsOffsetAllowed` test could be done before taking the monitor. But it's very minor, so totally optional.

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8926684 [details]
Bug 1415766. P1 - we never pass anything other than NS_SEEK_SET to Seek().

https://reviewboard.mozilla.org/r/197926/#review203126

> You don't really need `newOffset` anymore, you could just use `aOffset` throughout.

I see you've done that in the next patch, so nevermind. :-)

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8926685 [details]
Bug 1415766. P2 - move Seek() to private and tighten up some assertions.

https://reviewboard.mozilla.org/r/197928/#review203132

> As per my comment in the previous patch, the `IsOffsetAllowed` test could be done before taking the monitor. But it's very minor, so totally optional.

#2419 now asserts the monitor is already taken without taking it.

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

Thanks for the reviews!

Comment 11

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8926685 [details]
Bug 1415766. P2 - move Seek() to private and tighten up some assertions.

https://reviewboard.mozilla.org/r/197928/#review203132

> #2419 now asserts the monitor is already taken without taking it.

Oops, didn't notice, sorry. All good, then!

Comment 12

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8926685 [details]
Bug 1415766. P2 - move Seek() to private and tighten up some assertions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197928/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/753f3591930d
P1 - we never pass anything other than NS_SEEK_SET to Seek(). r=bechen,gerald
https://hg.mozilla.org/integration/autoland/rev/b12b1f6beb49
P2 - move Seek() to private and tighten up some assertions. r=bechen,gerald

Comment 14

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/753f3591930d
https://hg.mozilla.org/mozilla-central/rev/b12b1f6beb49