1105771 - H264 in ISOBMFF using AVC3 sample description cannot be played

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[u514836]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/39.0.2171.65 Safari/537.36

Steps to reproduce:

1. Navigate to http://dashif.org/reference/players/javascript/1.2.0/index.html
2. Enter http://rdmedia.bbc.co.uk/dash/ondemand/bbb/avc3/1/client_manifest-common_init.mpd in manifest box and click Load.


Actual results:

Video does not display - HTMLVideoElement.canPlayType returns "". Audio plays back as expected.


Expected results:

HTMLVideoElement should return "probably" for, and playback correctly, ISOBMFF content using AVC3 sample description (ie where the SPS/PPS are not in the initialization segment). An example codec string would be video/mp4;codecs="avc3.4d4015".

AVC3 is required to allow common initialization segments across all representations.

Comment 1

[Paul Silaghi, QA [:pauly]]

Confirmed in 37.0a1 (2014-12-05) Win 7

Comment 2

[Jean-Yves Avenard [:jya]]

Our decoders do not support avc3 at present. Could quite easily change FFmpeg h264 decoder to support it, but that won't get us very far...

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Retrieve SPS and PPS from AVCC stream when necessary

Extract SPS and PPS NAL from data stream and build extradata from it

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: MSE/MP4 Delay video decoder creation until we can retrieve SPS

Delay decoder creation until we have all the information to do so.

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Allow playback of avc3 streams

Add avc3 playback support

Comment 6

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8536243 [details] [diff] [review]
Retrieve SPS and PPS from AVCC stream when necessary

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

::: media/libstagefright/binding/AnnexB.cpp
@@ +80,5 @@
>  void
> +AnnexB::UpdateExtraData(mozilla::Vector<uint8_t>& aExtraData,
> +                        const MP4Sample* aSample)
> +{
> +  if (aExtraData[0] != 1 || !aSample->data) {

This should use ByteReader because it protects against all the run-time out of range conditions.

::: media/libstagefright/binding/DecoderData.cpp
@@ +192,5 @@
>  
> +void
> +VideoDecoderConfig::Update(const MP4Sample* aSample)
> +{
> +  if (extra_data.length() > 7) {

Now would be a good opportunity to move this logic and the above >=7 and |= 3 into the AnnexB class.

::: media/libstagefright/binding/mp4_demuxer.cpp
@@ +191,5 @@
>    }
>  
>    sample->Update(mVideoConfig.media_time);
> +  if (!mVideoConfig.CanInit()) {
> +    mVideoConfig.Update(sample);

Updating the video config seems like a nasty side-effect to have in DemuxVideoSample(). This could perhaps be solved by moving extra_data out of VideoConfig altogether.

Comment 7

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8536244 [details] [diff] [review]
MSE/MP4 Delay video decoder creation until we can retrieve SPS

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

The rest of this patch looks fine to me.

::: dom/media/fmp4/MP4Reader.cpp
@@ +433,5 @@
>        nsIntSize(video.display_width, video.display_height);
>      mVideo.mCallback = new DecoderCallback(this, kVideo);
> +    if (video.CanInit()) {
> +      nsresult rv = CreateVideoDecoder();
> +      NS_ENSURE_SUCCESS(rv, rv);

These lines seem unnecessary. We could always create the video decoder.

Comment 8

[Jean-Yves Avenard [:jya]]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #7)
> Comment on attachment 8536244 [details] [diff] [review]
> MSE/MP4 Delay video decoder creation until we can retrieve SPS
> 
> Review of attachment 8536244 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The rest of this patch looks fine to me.
> 
> ::: dom/media/fmp4/MP4Reader.cpp
> @@ +433,5 @@
> >        nsIntSize(video.display_width, video.display_height);
> >      mVideo.mCallback = new DecoderCallback(this, kVideo);
> > +    if (video.CanInit()) {
> > +      nsresult rv = CreateVideoDecoder();
> > +      NS_ENSURE_SUCCESS(rv, rv);
> 
> These lines seem unnecessary. We could always create the video decoder.

well, no, CreateVideoDecoder may fail.
NS_ENSURE_SUCCESS will return if rv isn't NS_OK.

rather confusing macro I admit, but it's how it was used

Comment 9

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Jean-Yves Avenard [:jya] from comment #8)
> > These lines seem unnecessary. We could always create the video decoder.
> 
> well, no, CreateVideoDecoder may fail.
> NS_ENSURE_SUCCESS will return if rv isn't NS_OK.
> 
> rather confusing macro I admit, but it's how it was used

Sorry - that was a typo. We should always create the video decoder lazily rather than having two execution paths, as discussed on IRC.

Comment 10

[Jean-Yves Avenard [:jya]]

Comment on attachment 8536243 [details] [diff] [review]
Retrieve SPS and PPS from AVCC stream when necessary

Will take a different approach.

It will be up to the decoder to handle the AVC format, not the demuxer

Comment 11

[Jean-Yves Avenard [:jya]]

Comment on attachment 8536244 [details] [diff] [review]
MSE/MP4 Delay video decoder creation until we can retrieve SPS

Will take a different approach.

It will be up to the decoder to handle the AVC format, not the demuxer

Comment 12

[Jean-Yves Avenard [:jya]]

(In reply to David Evans from comment #0)
> User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML,
> like Gecko) Chrome/39.0.2171.65 Safari/537.36
> 
> Steps to reproduce:
> 
> 1. Navigate to
> http://dashif.org/reference/players/javascript/1.2.0/index.html
> 2. Enter
> http://rdmedia.bbc.co.uk/dash/ondemand/bbb/avc3/1/client_manifest-
> common_init.mpd in manifest box and click Load.

David, I have this video playing now, but just like your other MSE sample, it has broken timestamps. The cts offset are marked as being unsigned (version 0 trun), yet are negative (bug 1105778).

Bug 1111311 addresses that problem, but still better following the specs.

Thanks

Comment 13

[u514836]

Awesome!

We are aware of this problem with these streams and we plan to reissue them with valid trun boxes very soon.

In the meantime, we have another test asset which should be valid: http://rdmedia.bbc.co.uk/dash/ondemand/testcard/1/client_manifest-events.mpd

(Note that this stream contains in-band DASH Events ('emsg' boxes) which the parser will need to ignore. If this is a problem, we will want to raise a bug for graceful support of those. In this case, an additional stream - http://rdmedia.bbc.co.uk/dash/ondemand/testcard/1/client_manifest.mpd - has the same media content without DASH events.)

Comment 14

[Jean-Yves Avenard [:jya]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ed08c1246e77

Comment 15

[u514836]

FYI, our Big Bucky Bunny MPEG-DASH reference streams have been reissued with valid trun boxes and other minor changes. Full details can be found at http://rdmedia.bbc.co.uk/dash/ondemand/bbb/, with the stream referenced in the user story now available at http://rdmedia.bbc.co.uk/dash/ondemand/bbb/2/client_manifest-common_init.mpd.

I see that Bug 1111311 has now landed in Nightly which makes this less of an issue but, as you say, still better to follow the spec.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Jean-Yves Avenard [:jya]]

For the record... As it is, the video will not have the right dimensions on mac until bug 1111328 lands.