Closed Bug 1105771 Opened 5 years ago Closed 5 years ago

H264 in ISOBMFF using AVC3 sample description cannot be played

Categories

(Core :: Audio/Video, defect)

36 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: u514836, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
Component: Untriaged → Video/Audio
Product: Firefox → Core
Blocks: MSE
Confirmed in 37.0a1 (2014-12-05) Win 7
Status: UNCONFIRMED → NEW
Ever confirmed: true
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...
See Also: → 1110534
See Also: → 1111328
Assignee: nobody → jyavenard
Extract SPS and PPS NAL from data stream and build extradata from it
Attachment #8536243 - Flags: review?(ajones)
Delay decoder creation until we have all the information to do so.
Attachment #8536244 - Flags: review?(ajones)
Add avc3 playback support
Attachment #8536245 - Flags: review?(cpearce)
Attachment #8536245 - Flags: review?(cpearce) → review+
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.
Attachment #8536243 - Flags: review?(ajones) → review-
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.
Attachment #8536244 - Flags: review?(ajones) → review+
(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
(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 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
Attachment #8536243 - Attachment is obsolete: true
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
Attachment #8536244 - Attachment is obsolete: true
Depends on: 1110534
See Also: 1110534
(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
Flags: needinfo?(bbcrddave)
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.)
Flags: needinfo?(bbcrddave)
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.
https://hg.mozilla.org/mozilla-central/rev/ed08c1246e77
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
For the record... As it is, the video will not have the right dimensions on mac until bug 1111328 lands.
You need to log in before you can comment on or make changes to this bug.