1355393 - Improve MediaCacheStream::NotifyDataReceived() by saving unnecessary copies

Status: RESOLVED FIXED

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

Description

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

http://searchfox.org/mozilla-central/rev/624d25b2980cf5f83452b040e6d664460f9b7ec5/dom/media/MediaCache.cpp#1821

By making FileBlockCache::WriteBlock() take 2 input buffers instead of one, we are able to write bytes in |PartialBlockBuffer| and |data| to the cache without copying |data| to |PartialBlockBuffer| first.

This allows us to save one copy in this case.

Comment 1

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

Attachment: Bug 1355393. P1 - let FileBlockCache::WriteBlock() take 2 buffers instead of one.

This allows us to save copies in combining 2 bufffers into one before calling
FileBlockCache::WriteBlock().

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

Comment 2

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

Attachment: Bug 1355393. P2 - write the bytes out when we've collected a whole block.

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

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8857296 [details]
Bug 1355393. P1 - let FileBlockCache::WriteBlock() take 2 buffers instead of one.

https://reviewboard.mozilla.org/r/129256/#review132290

::: dom/media/MediaCache.cpp:1818
(Diff revision 1)
>      }
>  
>      if (blockDataToStore) {
> -      gMediaCache->AllocateAndWriteBlock(this, blockDataToStore, mode);
> +      const uint8_t* p = reinterpret_cast<const uint8_t*>(blockDataToStore);
> +      auto data = MakeSpan<const uint8_t>(p, BLOCK_SIZE);
> +      gMediaCache->AllocateAndWriteBlock(this, mode, data, data.First(0));

Might be cleaner to have a polymorphic overloads of AllcoateAndWriteBlock() and WriteBlock() that takes only 1 span, and which pass MakeSpan<const uint8_t>(nullptr, 0) as the second argument to the two argument variant.

Comment 4

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

Comment on attachment 8857296 [details]
Bug 1355393. P1 - let FileBlockCache::WriteBlock() take 2 buffers instead of one.

https://reviewboard.mozilla.org/r/129256/#review132290

> Might be cleaner to have a polymorphic overloads of AllcoateAndWriteBlock() and WriteBlock() that takes only 1 span, and which pass MakeSpan<const uint8_t>(nullptr, 0) as the second argument to the two argument variant.

Default arguments should do the job. I will try to figure it out.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8857297 [details]
Bug 1355393. P2 - write the bytes out when we've collected a whole block.

https://reviewboard.mozilla.org/r/129258/#review132300

Comment 6

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

Comment on attachment 8857296 [details]
Bug 1355393. P1 - let FileBlockCache::WriteBlock() take 2 buffers instead of one.

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

Comment 7

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

Comment on attachment 8857297 [details]
Bug 1355393. P2 - write the bytes out when we've collected a whole block.

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

Comment 8

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

Thanks for the review!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/98b2e2843d5b
P1 - let FileBlockCache::WriteBlock() take 2 buffers instead of one. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/feb790f0853c
P2 - write the bytes out when we've collected a whole block. r=cpearce

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/98b2e2843d5b
https://hg.mozilla.org/mozilla-central/rev/feb790f0853c