Closed Bug 1370177 Opened 3 years ago Closed 3 years ago

Can't play video in www.phim.media

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: ayang, Assigned: cpearce)

Details

Attachments

(3 files)

It is ok at FF 53.0.3 on Mac.
Assignee: nobody → ayang
It fails at MoofParser::ScanForMetadata().

[MediaPDecoder #1]: D/MediaDemuxer MP4Demuxer(0x12e4340a0)::Init: MP4Demuxer Init()
[MediaPDecoder #1]: D/MediaDemuxer MoofParser(0x1082e2d80)::ScanForMetadata: source length 2782
[MediaPDecoder #1]: D/MediaDemuxer MoofParser(0x1082e2d80)::Metadata: No ftyp/moov

It can't find ftyp/moov.
The moov length is 0x1a83, 2782 is too short to parse the whole moov, I think this is reason of parsing failure.


If we disable E10S, it is ok.

[MediaPDecoder #1]: D/MediaDemuxer MP4Demuxer(0x1abb7baa0)::Init: MP4Demuxer Init()
[MediaPDecoder #1]: D/MediaDemuxer MoofParser(0x1b06df7c0)::ScanForMetadata: source length 15713
FileTypeBox { major_brand: isom, minor_version: 512, compatible_brands: [isom, iso2, avc1, mp41] }

The file length is difference when toggling E10S. Something could be wrong in MediaResource.
The incorrect length is from HttpBaseChannel::GetContentLength().

* thread #1, queue = 'com.apple.main-thread', stop reason = step over
    frame #0: 0x00000001077f4307 XUL`mozilla::ChannelMediaResource::OnStartRequest(this=0x00000001212d5c00, aRequest=0x0000000120929000) at MediaResource.cpp:236
   233 	    bool dataIsBounded = false;
   234
   235 	    int64_t contentLength = -1;
-> 236 	    hc->GetContentLength(&contentLength);
   237 	    if (contentLength >= 0 &&
   238 	        (responseStatus == HTTP_OK_CODE ||
   239 	         responseStatus == HTTP_PARTIAL_RESPONSE_CODE)) {


This problem happens only on streaming only, play in local is ok. https://goo.gl/U1Z1Xy
The correct length of this streaming content is 15,713 btw.
Assignee: ayang → nobody
Component: Audio/Video: Playback → Networking: HTTP
test.mp4 is the target resource.
2782 is the length of gzip'ed HTTP response body.
From HTTP code, looks like the size after unzip is stored in nsIHttpChannel.decodedBodySize.
https://searchfox.org/mozilla-central/rev/1a054419976437d0778a2b89be1b00207a744e94/netwerk/protocol/http/nsIHttpChannel.idl#121

Logically media need the size of uncompressed HTTP response body for compressed HTTP channel. I'm wondering why non-e10s can work.
Had an offline discussion with @alfredo and here is the summary:

1) media needs the actual size of the media file.
2) media will update the file size not only from GetContentLength but also from offset of OnDataAvailable
3) issue happens when OnDataAvailable comes late than ScanForMetadata, and the timing is changed recently.

Based these information, it's highly likely that the original media code doesn't handle compressed HTTP response well.
Here is the suggestion for determine the data size:

var data_length;

data_length = httpChannel.decodedBodySize;

if (data_length == 0) {
  data_length = httpChannel.contentLength;
}

// guessing content length for partial content afterward

@dragana please correct me if anything wrong about the nsIHttpChannel usage.
Flags: needinfo?(dd.mozilla)
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #7)
> Had an offline discussion with @alfredo and here is the summary:
> 
> 1) media needs the actual size of the media file.
> 2) media will update the file size not only from GetContentLength but also
> from offset of OnDataAvailable
> 3) issue happens when OnDataAvailable comes late than ScanForMetadata, and
> the timing is changed recently.
> 
> Based these information, it's highly likely that the original media code
> doesn't handle compressed HTTP response well.
> Here is the suggestion for determine the data size:
> 
> var data_length;
> 
> data_length = httpChannel.decodedBodySize;
> 
> if (data_length == 0) {
>   data_length = httpChannel.contentLength;
> }
> 
> // guessing content length for partial content afterward
> 
> @dragana please correct me if anything wrong about the nsIHttpChannel usage.

I am not sure I understand the problem completely. When is a decompressed body size needed, before OnDataAvailable?

decodedBodySize is only set in OnStopRequest.
Flags: needinfo?(dd.mozilla) → needinfo?(schien)
what is triggering ScanForMetadata?

We can update nsHttpChannel to update decodedBodySize on every OnDataAvailable, but that is not going to fix the problem.

nsHTTPCompressConv updates decodedBodySize only on OnDataAvailable, therefore if ScanForMetadata  is triggered before OnDataAvailable this will not fix the problem.
I somehow misread the nsHTTPCompressConv code and thought the OnStartRequest will be pending until decompression is completed. Sorry about that.

In this case, the only common timing to obtain the size of real data for both uncompressed and compressed HTTP response is OnDataAvailable.

@alfredo, is it possible to trigger ScanForMetadata until the first OnDataAvailable?
Flags: needinfo?(schien) → needinfo?(ayang)
To trigger ScanForMediadata until the first OnDataAvailable, it needs to change MDSM if my guess is correct.
Another way is to block in MediaCacheStream::GetLength() before the first OnDataAvailable().

I think @JW is more familiar to this part than me.
Flags: needinfo?(ayang) → needinfo?(jwwang)
MediaResource provides file-like APIs to downstream decoders by abstracting away the differences between channel-based resources and buffer-based resources. It is not feasible to delay reads until a particular event is received for the event might not fire at all on a buffer-based resource.

Is it possible to do some kind of sniffering (without decompressing the whole file) so correct file size can be obtained when OnStartRequest() is fired?
Flags: needinfo?(jwwang)
Or we could just have a length of -1 when the resource is compressed? That would mean we couldn't seek in the compressed stream, but we can't start decoding part way through a compressed resource anyway, as it would still be compressed, i.e. not a valid video file.

Seems a bit odd to compress a video stream; it's already compressed by the video codec!
If data is compressed we cannot decode anything. Anyway ScanForMetadata cannot scan(at least name say so) anything until OnDataAvailable is called, there is no data to search through (or am I wrong?)

MediaResource know whether it is channel-based resources and buffer-based resources? It can return WOULD_BLOCK if OnDataAvailable is not received yet, or it can have a callback API to inform when data is received.
Component: Networking: HTTP → Audio/Video
(In reply to Dragana Damjanovic [:dragana] from comment #14)
> If data is compressed we cannot decode anything. Anyway ScanForMetadata
> cannot scan(at least name say so) anything until OnDataAvailable is called,
> there is no data to search through (or am I wrong?)
> 
> MediaResource know whether it is channel-based resources and buffer-based
> resources? It can return WOULD_BLOCK if OnDataAvailable is not received yet,
> or it can have a callback API to inform when data is received.

It won't work. The 1st OnDataAvailable updates the length to 8316 which is still a wrong length. MediaResource have no idea how many OnDataAvailable events it has to wait before it can get a correct length.
Component: Audio/Video → Audio/Video: Playback
Dragana and I spoke at MozSF. Necko's decompressor is streaming; as it receives compressed chunks it decompresses them and then forwards them onto our OnDataAvailable listener. So Necko doesn't know the decompressed size up front. Potentially it's in metadata in the compressed stream, but it's not safe to trust metadata. We don't want to wait for the entire stream to download and decompress so we know the decompressed size before beginning playback.

We can't sensibly seek in compressed HTTP channels because we don't know the length. So we decided the best path forward was to detect when HTTP channels are compressed, and treat them as if they're infinite and have unseekable transports. This will mean that we can play them and we will be able to seek our demuxers inside buffered decompressed ranges, and we won't try to seek into unbuffered ranges which would still be compressed.

We can detect whether a stream is compressed by the presence of a "Content-Encoding" HTTP header. We already turn off compression on our HTTP requests for media in HTMLMediaElement::SetRequestHeaders():
https://searchfox.org/mozilla-central/rev/a3a739de04ee6134c11546568a33dbb6a6a29907/dom/html/HTMLMediaElement.cpp#6545

In the example at http://www.phim.media/test.mp4 our media stack will be passed an already setup channel from the document viewer/sniffer. So it will be too late to change the headers on that HTTP channel, in that case we'll get "Content-Encoding: gzip" irrespective of the code in HTMLMediaElement::SetRequestHeaders().

When testing the http://www.phim.media/test.mp4 test case, be aware that the file may be cached by Necko or an intermediary cache; if the response is 304, the length will be known and you'll need to CTRL+SHIFT+R to force a cache invalidation so that you get the compressed 200 response, which will fail.
Assignee: nobody → cpearce
Comment on attachment 8883266 [details]
Bug 1370177 - Add test case for gzipped MP4 with Content-Length set to compressed file length. .

https://reviewboard.mozilla.org/r/154200/#review159378

::: dom/media/test/gzipped_mp4.sjs:15
(Diff revision 1)
> +    Components.classes['@mozilla.org/network/file-input-stream;1']
> +              .createInstance(Components.interfaces.nsIFileInputStream);
> +  var binaryInputStream =
> +    Components.classes["@mozilla.org/binaryinputstream;1"]
> +              .createInstance(Components.interfaces.nsIBinaryInputStream);
> +  fileInputStream.init(file, -1, -1, false);

The last parameter should be 0 instead of false.

::: dom/media/test/test_video_gzip_encoding.html:15
(Diff revision 1)
> +    <!--
> +      Tests that an MP4 file served over a "Content-Encoding: gzip"
> +      HTTP channel with a "Content-Length" header set to the length
> +      of the compressed file works.
> +    -->
> +    <video id='v' src="http://mochi.test:8888/tests/dom/media/test/gzipped_mp4.sjs" controls autoplay></video>

It would be nice to add 'gzipped_mp4.sjs' to gPlayTests in manifest.js so we can reuse test_playback.html
Attachment #8883266 - Flags: review?(jwwang) → review+
Comment on attachment 8883267 [details]
Bug 1370177 - Treat compressed HTTP channels as having infinite length and as unseekable in media code. .

https://reviewboard.mozilla.org/r/154202/#review159384

::: dom/media/MediaResource.cpp:337
(Diff revision 1)
>        dataIsBounded = true;
>      }
>  
>      mCallback->SetInfinite(!dataIsBounded);
>    }
>    mCacheStream.SetTransportSeekable(seekable);

We should set non-seekable when isCompressed is true, right?
Attachment #8883267 - Flags: review?(jwwang) → review+
Comment on attachment 8883267 [details]
Bug 1370177 - Treat compressed HTTP channels as having infinite length and as unseekable in media code. .

https://reviewboard.mozilla.org/r/154202/#review159404

::: dom/media/MediaResource.cpp:337
(Diff revision 1)
>        dataIsBounded = true;
>      }
>  
>      mCallback->SetInfinite(!dataIsBounded);
>    }
>    mCacheStream.SetTransportSeekable(seekable);

Yes, good catch.
Comment on attachment 8883266 [details]
Bug 1370177 - Add test case for gzipped MP4 with Content-Length set to compressed file length. .

https://reviewboard.mozilla.org/r/154200/#review159378

> It would be nice to add 'gzipped_mp4.sjs' to gPlayTests in manifest.js so we can reuse test_playback.html

The MP4 demuxer can't handle determining the duration of the stream until it knows the length. So if we add this case to test_playback.html, we fail the duration check in checkMetadata() in loadedmetadata. So it fails in test_playback.
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a4377a5a1ff6
Add test case for gzipped MP4 with Content-Length set to compressed file length. r=jwwang.
https://hg.mozilla.org/integration/autoland/rev/f10af89ca17a
Treat compressed HTTP channels as having infinite length and as unseekable in media code. r=jwwang.
Comment on attachment 8883266 [details]
Bug 1370177 - Add test case for gzipped MP4 with Content-Length set to compressed file length. .

https://reviewboard.mozilla.org/r/154200/#review159378

> The MP4 demuxer can't handle determining the duration of the stream until it knows the length. So if we add this case to test_playback.html, we fail the duration check in checkMetadata() in loadedmetadata. So it fails in test_playback.

http://searchfox.org/mozilla-central/rev/e8f4f51cd543f203e9cb861cecb7545ac43c836c/dom/media/test/manifest.js#260
We have this entry where duration is not specified so checkMetadata() won't check the duration.
https://hg.mozilla.org/mozilla-central/rev/a4377a5a1ff6
https://hg.mozilla.org/mozilla-central/rev/f10af89ca17a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment on attachment 8883267 [details]
Bug 1370177 - Treat compressed HTTP channels as having infinite length and as unseekable in media code. .

Approval Request Comment
[Feature/Bug causing the regression]:unknown
[User impact if declined]:decoded error encountered when compressed content is serviced by the server.
[Is this code covered by automated tests?]:yes
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:1373618
[Is the change risky?]:low
[Why is the change risky/not risky?]:the change is simple and we have a test case to ensure no regression is introduced.
[String changes made/needed]:none
Attachment #8883267 - Flags: approval-mozilla-beta?
Comment on attachment 8883267 [details]
Bug 1370177 - Treat compressed HTTP channels as having infinite length and as unseekable in media code. .

handle compressed media, beta55+
Attachment #8883267 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #29)
> [Is this code covered by automated tests?]:yes
> [Has the fix been verified in Nightly?]:yes
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on JW Wang's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.