Closed Bug 1412737 Opened 2 years ago Closed 2 years ago

MediaCacheStream::Read() should read enough bytes until EOS or error is encounted

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(3 files, 1 obsolete file)

The caller of MediaCacheStream::Read() will try again until EOS or error is returned. We should encode this logic in MediaCacheStream::Read() so the caller code can be simplified.
Assignee: nobody → jwwang
Blocks: 1369263
Priority: -- → P3
Attachment #8925764 - Flags: review?(bechen)
Attachment #8925765 - Flags: review?(bechen)
Attachment #8925766 - Flags: review?(bechen)
Attachment #8925767 - Flags: review?(bechen)
Comment on attachment 8925764 [details]
Bug 1412737. P1 - improve error handling.

https://reviewboard.mozilla.org/r/196934/#review202106
Attachment #8925764 - Flags: review?(bechen) → review+
Comment on attachment 8925765 [details]
Bug 1412737. P2 - Read() should return only when enough bytes are read or EOS/error is encountered.

https://reviewboard.mozilla.org/r/196936/#review202130

::: dom/media/MediaCache.cpp:2529
(Diff revision 1)
>        MediaCache::ResourceStreamIterator iter(mMediaCache, mResourceID);
>        while (MediaCacheStream* stream = iter.Next()) {
>          if (OffsetToBlockIndexUnchecked(stream->mChannelOffset) ==
>                streamBlock &&
> -            streamOffset < stream->mChannelOffset) {
> +            streamOffset < stream->mChannelOffset &&
> +            stream->mChannelOffset == stream->mStreamLength) {

I have a question about part2 and part3 patches, what happened if the download speed is equal to consumed speed and also the mStreamLength(total length) doesn't set?
Comment on attachment 8925765 [details]
Bug 1412737. P2 - Read() should return only when enough bytes are read or EOS/error is encountered.

https://reviewboard.mozilla.org/r/196936/#review202130

> I have a question about part2 and part3 patches, what happened if the download speed is equal to consumed speed and also the mStreamLength(total length) doesn't set?

|mStreamLength == -1| means the total length is unknown and |stream->mChannelOffset == stream->mStreamLength| will never be true since mChannelOffset is always non-negative. Therefore we will never read data from the partial block, instead we will always wait for the block to commit to the cache and read data from the cache.
Comment on attachment 8925765 [details]
Bug 1412737. P2 - Read() should return only when enough bytes are read or EOS/error is encountered.

https://reviewboard.mozilla.org/r/196936/#review202148
Attachment #8925765 - Flags: review?(bechen) → review+
Comment on attachment 8925766 [details]
Bug 1412737. P3 - remove the while loops in the callers of MediaCacheStream::Read().

https://reviewboard.mozilla.org/r/196938/#review202150
Attachment #8925766 - Flags: review?(bechen) → review+
Comment on attachment 8925767 [details]
Bug 1412737. P4 - wake up readers only when we have blocks committed to the cache.

https://reviewboard.mozilla.org/r/196940/#review202152
Attachment #8925767 - Flags: review?(bechen) → review+
Attachment #8925764 - Flags: review?(gsquelart)
Attachment #8925765 - Flags: review?(gsquelart)
Attachment #8925766 - Flags: review?(gsquelart)
Attachment #8925767 - Flags: review?(gsquelart)
Comment on attachment 8925764 [details]
Bug 1412737. P1 - improve error handling.

https://reviewboard.mozilla.org/r/196934/#review202632

::: dom/media/MediaCache.cpp:2490
(Diff revision 1)
>  
>    uint32_t count = 0;
>    // Read one block (or part of a block) at a time
>    while (count < aCount) {
> +    if (mClosed) {
> +      return NS_ERROR_ABORT;

You've changed the previous NS_ERROR_FAILURE's into NS_ERROR_ABORT, is that intended?
Attachment #8925764 - Flags: review?(gsquelart) → review+
Comment on attachment 8925764 [details]
Bug 1412737. P1 - improve error handling.

https://reviewboard.mozilla.org/r/196934/#review202632

> You've changed the previous NS_ERROR_FAILURE's into NS_ERROR_ABORT, is that intended?

Yes.

NS_ERROR_ABORT (0x80004004)
    This error indicates that an operation failed and the caller should abort whatever action is being performed. This typically will occur if an operation could not complete properly.

NS_ERROR_ABORT is a more specific error code which better describes Read() failed due to shutdown.
Comment on attachment 8925765 [details]
Bug 1412737. P2 - Read() should return only when enough bytes are read or EOS/error is encountered.

https://reviewboard.mozilla.org/r/196936/#review202638

::: dom/media/MediaCache.cpp:2585
(Diff revision 1)
> -  if (count > 0) {
> +  if (count == 0) {
> +    return NS_OK;

This early return means you won't set *aBytes in this case, and at least one caller (MediaResourceIndex::MediaReadAt) does not initialize that variable!

At the minimum you could add a `MOZ_ASSERT(*aBytes == 0)` to catch missing external initializations; otherwise it would be safer to add `*aBytes = 0;` here.
Attachment #8925765 - Flags: review?(gsquelart) → review+
Comment on attachment 8925765 [details]
Bug 1412737. P2 - Read() should return only when enough bytes are read or EOS/error is encountered.

https://reviewboard.mozilla.org/r/196936/#review202638

> This early return means you won't set *aBytes in this case, and at least one caller (MediaResourceIndex::MediaReadAt) does not initialize that variable!
> 
> At the minimum you could add a `MOZ_ASSERT(*aBytes == 0)` to catch missing external initializations; otherwise it would be safer to add `*aBytes = 0;` here.

Good catch! I will do |*aBytes = 0|.
Comment on attachment 8925766 [details]
Bug 1412737. P3 - remove the while loops in the callers of MediaCacheStream::Read().

https://reviewboard.mozilla.org/r/196938/#review202646

::: dom/media/MediaResource.cpp:424
(Diff revision 1)
> +  uint32_t bytesRead = 0;
> +  nsresult rv = mResource->ReadAt(aOffset, aBuffer, aCount, &bytesRead);
> +  if (NS_SUCCEEDED(rv)) {
> +    *aBytes = bytesRead;

As a tiny optimization (but more importantly, it would remove lots of line -- less to read & maintain), you could just pass aBytes to ReadAt.

(and same below)
Attachment #8925766 - Flags: review?(gsquelart) → review+
Comment on attachment 8925766 [details]
Bug 1412737. P3 - remove the while loops in the callers of MediaCacheStream::Read().

https://reviewboard.mozilla.org/r/196938/#review202646

> As a tiny optimization (but more importantly, it would remove lots of line -- less to read & maintain), you could just pass aBytes to ReadAt.
> 
> (and same below)

Good idea! Thanks for the notice.
Comment on attachment 8925767 [details]
Bug 1412737. P4 - wake up readers only when we have blocks committed to the cache.

https://reviewboard.mozilla.org/r/196940/#review202648

::: commit-message-db58b:3
(Diff revision 1)
> +Bug 1412737. P4 - wake up readers only when we have blocks committed to the cache.
> +
> +Per P2 chagnes, a reader will always read data from the cache or from the last

'chagnes' -> 'changes'

::: dom/media/MediaCache.cpp:2071
(Diff revision 1)
>    // XXX it would be fairly easy to optimize things a lot more to
>    // avoid waking up reader threads unnecessarily

Wouldn't you say you have just addressed this comment? ;-)
Attachment #8925767 - Flags: review?(gsquelart) → review+
Comment on attachment 8925767 [details]
Bug 1412737. P4 - wake up readers only when we have blocks committed to the cache.

https://reviewboard.mozilla.org/r/196940/#review202648

> Wouldn't you say you have just addressed this comment? ;-)

I think there is still room for improvement. So I leave the todo item there. :)
Thanks for the reviews!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/29e511fbcd62
P1 - improve error handling. r=bechen,gerald
https://hg.mozilla.org/integration/autoland/rev/dd35b8813ca1
P2 - Read() should return only when enough bytes are read or EOS/error is encountered. r=bechen,gerald
https://hg.mozilla.org/integration/autoland/rev/b8ae4f1e89c9
P3 - remove the while loops in the callers of MediaCacheStream::Read(). r=bechen,gerald
https://hg.mozilla.org/integration/autoland/rev/13b3569d56c4
P4 - wake up readers only when we have blocks committed to the cache. r=bechen,gerald
Comment on attachment 8925764 [details]
Bug 1412737. P1 - improve error handling.

https://reviewboard.mozilla.org/r/196934/#review202672


C/C++ static analysis found 0 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8925765 [details]
Bug 1412737. P2 - Read() should return only when enough bytes are read or EOS/error is encountered.

https://reviewboard.mozilla.org/r/196936/#review202676


C/C++ static analysis found 0 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8925766 [details]
Bug 1412737. P3 - remove the while loops in the callers of MediaCacheStream::Read().

https://reviewboard.mozilla.org/r/196938/#review202680


C/C++ static analysis found 0 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/3f797c2aedfb
Backed out 4 changesets for frequent Windows talos-g4 timeouts. a=RyanVM
Backed out for frequent Windows talos-g4 timeouts:

https://hg.mozilla.org/mozilla-central/rev/3f797c2aedfb7be631be963e1a402fa9054076c7

Push range with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&fromchange=270e0b97ec67b236515a1487fdab7a153f15d65f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=success&filter-searchStr=T-e10s%28g4%29&tochange=ac109e443ac2facf708848a6f95a5a1e553596ed&selectedJob=142921637
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=142921637&repo=autoland

19:56:34     INFO -  PID 1732 | testsrc.1080p.60fps.mp4_scale_2_startup = 8.022660427807482 ms/frame
19:56:34     INFO -  PID 1732 |
19:56:34     INFO -  PID 1732 | testsrc.1080p.60fps.mp4_scale_2_inclip = 7.5760227272727265 ms/frame
19:56:34     INFO -  PID 1732 |
19:56:34     INFO -  PID 1732 | Cycle 1(3): loaded http://localhost:49756/tests/video/video_playback.html (next: http://localhost:49756/tests/video/video_playback.html)
20:00:19     INFO -  PID 1732 | [Parent 1732, Gecko_IOThread] WARNING: pipe error: 109: file z:/build/build/src/ipc/chromium/src/chrome/common/ipc_channel_win.cc, line 346

command timed out: 3600 seconds without output, attempting to kill
21:00:19     INFO - Automation Error: mozprocess timed out after 3600 seconds running ['C:\\slave\\test\\build\\venv\\Scripts\\python', 'C:\\slave\\test\\build\\tests\\talos\\talos\\run_tests.py', '--branchName', 'Autoland-Non-PGO', '--suite', 'g4-e10s', '--executablePath', 'C:\\slave\\test\\build\\application\\firefox\\firefox', '--symbolsPath', 'https://queue.taskcluster.net/v1/task/AQz1dlUGToaG_35t51qB0A/artifacts/public/build/target.crashreporter-symbols.zip', '--title', 'T-W732-IX-086', '--webServer', 'localhost', '--log-tbpl-level=debug', '--log-errorsummary=C:\\slave\\test\\build\\blobber_upload_dir\\g4-e10s_errorsummary.log', '--log-raw=C:\\slave\\test\\build\\blobber_upload_dir\\g4-e10s_raw.log']
SIGKILL failed to kill process
using fake rc=-1
program finished with exit code -1
Flags: needinfo?(jwwang)
Target Milestone: mozilla58 → ---
It looks like the optimization in P4 sometimes fails to wake up the reader and results in MediaCacheStream::Read() never returns.

Since P4 is an optimization nice but not necessary to have, I will land P1-P3 and open a new bug to investigate the issue in P4.
Flags: needinfo?(jwwang)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8925767 - Attachment is obsolete: true
Blocks: 1416084
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5a5808da985b
P1 - improve error handling. r=bechen,gerald
https://hg.mozilla.org/integration/autoland/rev/39f2cccbf9f7
P2 - Read() should return only when enough bytes are read or EOS/error is encountered. r=bechen,gerald
https://hg.mozilla.org/integration/autoland/rev/95b93405af2b
P3 - remove the while loops in the callers of MediaCacheStream::Read(). r=bechen,gerald
https://hg.mozilla.org/mozilla-central/rev/5a5808da985b
https://hg.mozilla.org/mozilla-central/rev/39f2cccbf9f7
https://hg.mozilla.org/mozilla-central/rev/95b93405af2b
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1424838
No longer depends on: 1424838
You need to log in before you can comment on or make changes to this bug.