1404771 - Some MediaCache code cleanup

Status: RESOLVED FIXED

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

Comment 1

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

Attachment: Bug 1404771. P1 - fix the comment of mStreamLength.

mStreamLength is always accessed within the lock. So it is safe to read/write
mStreamLength on all threads.

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

Comment 2

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

Attachment: Bug 1404771. P2 - always access mThrottleReadahead within the lock.

So it is easier to run Update() loops off the main thread in the future.

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

Comment 3

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

Attachment: Bug 1404771. P3 - constify some members and fix comments.

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

Comment 4

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

Comment on attachment 8914153 [details]
Bug 1404771. P1 - fix the comment of mStreamLength.

https://reviewboard.mozilla.org/r/185472/#review190388

Comment 5

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

Comment on attachment 8914154 [details]
Bug 1404771. P2 - always access mThrottleReadahead within the lock.

https://reviewboard.mozilla.org/r/185474/#review190390

Comment 6

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

Comment on attachment 8914155 [details]
Bug 1404771. P3 - constify some members and fix comments.

https://reviewboard.mozilla.org/r/185476/#review190396

::: dom/media/MediaCache.h:465
(Diff revision 1)
>    // than the priority of the data already in the cache
>    bool mCacheSuspended;
>    // True if the channel ended and we haven't seeked it again.
>    bool mChannelEnded;
>  
>    // The following fields are protected by the cache's monitor can can be written

While you're here, you could fix this comment: I think the first 'can' should be 'and'.

::: dom/media/MediaCache.h:509
(Diff revision 1)
> -  UniquePtr<uint8_t[]> mPartialBlockBuffer = MakeUnique<uint8_t[]>(BLOCK_SIZE);
> +  // This partial buffer should always be read/write within the cache's monitor.
> +  const UniquePtr<uint8_t[]> mPartialBlockBuffer =
> +    MakeUnique<uint8_t[]>(BLOCK_SIZE);

Noticed while reviewing this: This field and a few others seem to be accessed without lock in MediaCacheStream::SizeOfExcludingThis, is that acceptable?

Comment 7

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

Comment on attachment 8914155 [details]
Bug 1404771. P3 - constify some members and fix comments.

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

Comment 8

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

Comment on attachment 8914155 [details]
Bug 1404771. P3 - constify some members and fix comments.

https://reviewboard.mozilla.org/r/185476/#review190396

> Noticed while reviewing this: This field and a few others seem to be accessed without lock in MediaCacheStream::SizeOfExcludingThis, is that acceptable?

mPartialBlockBuffer.get() is immutable after construction so it can be safely accessed without any lock. However, the buffer pointed by mPartialBlockBuffer.get() must be protected by the lock.

Comment 9

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

Thanks for the review!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/edb79608b20b
P1 - fix the comment of mStreamLength. r=gerald
https://hg.mozilla.org/integration/autoland/rev/08258a04521e
P2 - always access mThrottleReadahead within the lock. r=gerald
https://hg.mozilla.org/integration/autoland/rev/7c06e30b1373
P3 - constify some members and fix comments. r=gerald

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/edb79608b20b
https://hg.mozilla.org/mozilla-central/rev/08258a04521e
https://hg.mozilla.org/mozilla-central/rev/7c06e30b1373