993598 - POST of MediaRecorder Blob drops first byte with long video slices

Status: RESOLVED FIXED

(Core :: Audio/Video: Recording, defect)

Description

[Andreas Girgensohn]

Attachment: Test case

When recording video with MediaRecorder and uploading the Blob with xhr POST, the first byte is dropped for long video slices (2 seconds works correctly, 20 seconds does not).  In the sequence 1a 45 df a3, 1a is dropped.  The missing byte is included in byte count reported in the content-length header of the request.  I verified this by running WireShark on the HTTP traffic.

When calling FileReader.readAsArrayBuffer on the Blob and uploading the ArrayBuffer instead, the missing byte is included.

The video camera is a Logitech HD Pro Webcam C920 in case that makes a difference.

Comment 1

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Hi Benjamen, 
Could you help to check this problem?

Comment 2

[Benjamin Chen [:bechen]]

We can reproduce the bug now include the 2s/20s blobs have different behaviors.

Vincent:
Should we move the bug to Necko?

Comment 3

[Benjamin Chen [:bechen]]

Update status:
There is a data byte threshold in EncodedBufferCache.
If the data size accumulated over the threshold, we will create nsDOMTemporaryFileBlob, otherwise we create a nsDOMMemoryFile. 
I think that causes different behaviours for 2s and 20s blobs.

About the missing 1 byte, now we suspect the macro |NS_InputStreamIsBuffered| somehow it will read 1 byte of the blob.
http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp?from=nsxmlhttprequest.cpp#2732

Comment 4

[Benjamin Chen [:bechen]]

Here read 1 byte.
http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp?from=nsxmlhttprequest.cpp#2732

And then 
http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsDOMFile.cpp?from=nsDOMFile.cpp&case=true#792
or
http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsDOMFile.cpp?from=nsDOMFile.cpp&case=true#800

using mStarPos (now should be 1) instead the original start point mStart.

Comment 5

[Benjamin Chen [:bechen]]

|nsDOMMemoryFile::GetInternalStream| retruns a seekable stream.

|nsDOMTemporaryFileBlob::GetInternalStream| returns a non-seekable stream.

Now I suspect the |nsTemporaryFileInputStream| does not implement nsISeekableStream.

Comment 6

[Benjamin Chen [:bechen]]

Root cause:
1. http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp?from=nsxmlhttprequest.cpp#2732
2. http://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsStreamUtils.cpp#671
3. http://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsStreamUtils.cpp#658

The |TestInputStream| returns a NS_ERROR_ABORT.

And 
http://dxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsTemporaryFileInputStream.cpp#81
moving forward the mStartPos regardless the return value.

Comment 7

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

OK, thanks, please help to fix it. :)

Comment 8

[Benjamin Chen [:bechen]]

Attachment: bug-993598.patch

Comment 9

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Comment on attachment 8420806 [details] [diff] [review]
bug-993598.patch

Review of attachment 8420806 [details] [diff] [review]:
-----------------------------------------------------------------

Could we have a test case for recording more than 20s without passing timeslice?

::: netwerk/base/src/nsTemporaryFileInputStream.cpp
@@ +73,5 @@
>      uint32_t write_result = 0;
>      nsresult rv = writer(this, closure, buf,
>                           count - remainBufCount, bufCount, &write_result);
>      remainBufCount -= bufCount;
> +    if (NS_FAILED(rv)) {

You can use NS_ENSURE_SUCCESS(res, ret)

Comment 10

[Benjamin Chen [:bechen]]

(In reply to Randy Lin [:rlin] from comment #9)
> Comment on attachment 8420806 [details] [diff] [review]
> bug-993598.patch
> 
> Review of attachment 8420806 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Could we have a test case for recording more than 20s without passing
> timeslice?
Yes, bug 1008797 will do it. We may add more network tests there.

Comment 11

[Benjamin Chen [:bechen]]

Attachment: bug-993598.patch

Comment 12

[Benjamin Chen [:bechen]]

Attachment: bug-993598.patch

r=roc
try server: https://tbpl.mozilla.org/?tree=Try&rev=558497feb80c

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cfb4a97f2fbf

Comment 14

[Benjamin Chen [:bechen]]

Randy: 
Do we need to uplift the patch to 1.2 1.3 1.4 etc...?

Comment 15

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

1.4 may need this.

Comment 16

[Andreas Girgensohn]

As I'm getting all these email notifications, I had a look at the code being patched. That code assumes that PR_Read always reads as many bytes as asked as long as there isn't an error.  In my opinion, it would be better to write read_result bytes and advance mStartPos by that much.  If you also wanted to handle writer writing less than asked, you would need an inner loop.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cfb4a97f2fbf