1414709 - Use Span<> to replace low level pointer arithmetic in MediaCacheStream::NotifyDataReceived()

Status: RESOLVED FIXED

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

Comment 1

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

Attachment: Bug 1414709 - use Span<> to replace low level pointer arithmetic in MediaCacheStream::NotifyDataReceived().

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

Comment 2

[Benjamin Chen [:bechen]]

Comment on attachment 8925411 [details]
Bug 1414709 - use Span<> to replace low level pointer arithmetic in MediaCacheStream::NotifyDataReceived().

https://reviewboard.mozilla.org/r/196532/#review201746

Comment 3

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

Comment on attachment 8925411 [details]
Bug 1414709 - use Span<> to replace low level pointer arithmetic in MediaCacheStream::NotifyDataReceived().

https://reviewboard.mozilla.org/r/196532/#review201766

::: dom/media/ChannelMediaResource.cpp:443
(Diff revision 1)
>  {
>    Closure* closure = static_cast<Closure*>(aClosure);
>    closure->mResource->mCacheStream.NotifyDataReceived(
> -    closure->mLoadID, aCount, aFromSegment);
> +    closure->mLoadID,
> +    aCount,
> +    reinterpret_cast<const unsigned char*>(aFromSegment));

'unsigned char' -> 'uint8_t', to match the NotifyDataReceived argument.

Comment 4

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

Comment on attachment 8925411 [details]
Bug 1414709 - use Span<> to replace low level pointer arithmetic in MediaCacheStream::NotifyDataReceived().

https://reviewboard.mozilla.org/r/196532/#review201766

> 'unsigned char' -> 'uint8_t', to match the NotifyDataReceived argument.

That was done intentionally. AFAIK, uint8_t doesn't always equal 'unsigned char' on all platforms and casting char* to uint8_t* will break strict aliasing if sizeof(uint8_t) != sizeof(char).

Comment 5

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

I guess strict aliasing is not an issue here since we never dereference |aData|.

Comment 6

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

Comment on attachment 8925411 [details]
Bug 1414709 - use Span<> to replace low level pointer arithmetic in MediaCacheStream::NotifyDataReceived().

https://reviewboard.mozilla.org/r/196532/#review201766

> That was done intentionally. AFAIK, uint8_t doesn't always equal 'unsigned char' on all platforms and casting char* to uint8_t* will break strict aliasing if sizeof(uint8_t) != sizeof(char).

I thought aliasing between any type and `char` was supported?
And then you call a function that takes `const uint8_t*`, so there's still a potential cast from `const unsigned char*` to `const uint8_t*` (if they are in fact different).

Comment 7

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

Comment on attachment 8925411 [details]
Bug 1414709 - use Span<> to replace low level pointer arithmetic in MediaCacheStream::NotifyDataReceived().

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

Comment 8

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

Thanks for the reviews!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5915590f9500
use Span<> to replace low level pointer arithmetic in MediaCacheStream::NotifyDataReceived(). r=bechen,gerald

Comment 10

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/5915590f9500