Closed Bug 1355393 Opened 5 years ago Closed 5 years ago

Improve MediaCacheStream::NotifyDataReceived() by saving unnecessary copies

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
Assignee: nobody → jwwang
Priority: -- → P3
Blocks: 1355409
Attachment #8857296 - Flags: review?(cpearce)
Attachment #8857297 - Flags: review?(cpearce)
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.
Attachment #8857296 - Flags: review?(cpearce) → review+
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 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
Attachment #8857297 - Flags: review?(cpearce) → review+
Thanks for the review!
Pushed by jwwang@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/98b2e2843d5b
https://hg.mozilla.org/mozilla-central/rev/feb790f0853c
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.