938512 - [RTSP] HTMLMediaElement does not fire loadedmetadata event for RTSP streaming.

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Ethan Tseng [:ethan]]

In the case of RTSP streaming in video element, the "preload" attribute does not work as expected.
For example, consider the following HTML:
<video id=home_video controls preload=metadata width="100%">
  <source src="rtsp://184.72.239.149/vod/mp4:BigBuckBunny_115k.mov"/> 
</video>

While preload attribute is assigned as "metadata", HTMLMediaElement should dispatch asynchronous event "loadedmetadata", but it does not. It will be triggered until the user click the "Play" button on the control panel.

Comment 1

[Ethan Tseng [:ethan]]

Attachment: bug-938512-fix.patch

When RTSP connection is establised (which means RTSP channel is created), the playback is ready to start. We should not pause it.

Comment 2

[Ethan Tseng [:ethan]]

*** Work Note ***
When media decoder thread is started, it enters the function "MediaDecoder::MetadataLoaded()".
In AbstractMediaDecoder.h:
  NS_IMETHOD Run() MOZ_OVERRIDE
  {
    mDecoder->MetadataLoaded(mChannels, mRate, mHasAudio, mHasVideo, mTags);
    return NS_OK;
  }
This function calls another MetadataLoaded() of the media element owner.
In MediaDecoder.cpp
  mOwner->MetadataLoaded(aChannels, aRate, aHasAudio, aHasVideo, aTags);

We verified this is called for both normal and rtsp video elements.

The call flow of Decoder State Machine:
MediaDecoderStateMachine::RunStateMachine()
-> MediaDecoderStateMachine::ScheduleDecodeThread()

MediaDecoderStateMachine::DecodeThreadRun()
-> MediaDecoderStateMachine::DecodeMetadata
  -> MediaOmxReader::ReadMetadata()
   -> mOmxDecoder->TryLoad()

The key point is, in rtsp case, mOmxDecoder->TryLoad() calls read() and waits for data pumping into the buffer. And this would happen when "play" button is clicked to trigger the RTP dataflow. This is why rtsp video element does not fire loadedmetadata event before playing.

We should figure out a solution for this.

Comment 3

[Ethan Tseng [:ethan]]

*** Work Note ***
Rtsp media element obtains metadata while rtsp connection is OnConnected.
However, the decoder thread itself is blocked to wait the first audio/video frame pumping into the buffer, which is triggered by the user play action to start to transfer RTP streaming data.

For audio: MediaDecoderStateMachine::DecodeMetadata() ==> mReader->ReadMetadata() ==> mOmxDecoder->TryLoad()
For video: MediaDecoderStateMachine::FindStartTime() ==> mReader->FindStartTime() ==> 
           MediaDecoderReader::FindStartTime ==> MediaDecoderReader::DecodeToFirstVideoData()

Apparently this issue originates from the different design between HTTP and RTSP media resources.
Maybe we can try to fire the event manually in RTSP media resource.
In RtspMediaResource::OnConnected(), call HTMLMediaElement::MetadataLoaded().

Comment 4

[Ethan Tseng [:ethan]]

Attachment: bug-938512-fix.patch

To resolve metadata loading issue for RTSP protocol.
In OmxDecoder::TryLoad(), if media source is rtsp, we don't try to read the audio source.
In RtspOmxReader that inherits MediaOmxReader, we override FindStartTime() to skip getting the first video frame.
With this patch, rtsp media element would successfully triggers "loadedmetadata" event up to Gaia.

Comment 5

[Ethan Tseng [:ethan]]

Attachment: MediaElementStateMachine.png

State machine diagram of media coder.
This diagram is depicted from the comments in content/media/MediaDecoder.h.

Comment 6

[Ethan Tseng [:ethan]]

Attachment: MediaElementStateMachine.png

Comment 7

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8338381 [details] [diff] [review]
bug-938512-fix.patch

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

This looks ok, but I'd like you to confirm some things. If we're going to fire loadedmetadata, I'd like it to be clear what metadata we expect to be available, and how that can be obtained. For example, if you can get metadata at onconnected, please set as many variables as you can in MediaDecoderStateMachine.

::: content/media/omx/RtspOmxReader.h
@@ +68,5 @@
> +  // DECODING state.
> +  virtual VideoData* FindStartTime(int64_t& aOutStartTime)
> +    MOZ_FINAL MOZ_OVERRIDE {
> +    return nullptr;
> +  }

This means mStartTime will be set to 0 in MediaDecoderStateMachine::FindStartTime, and mEndTime will not be set. Since OnConnected has happened, do you have a start time or end time in the metadata? Can you access those in FindStartTime?

Comment 8

[Ethan Tseng [:ethan]]

(In reply to Steve Workman [:sworkman] from comment #7)
> Review of attachment 8338381 [details] [diff] [review]:
> -----------------------------------------------------------------
> This looks ok, but I'd like you to confirm some things. If we're going to
> fire loadedmetadata, I'd like it to be clear what metadata we expect to be
> available, and how that can be obtained. For example, if you can get
> metadata at onconnected, please set as many variables as you can in
> MediaDecoderStateMachine.
From the perspective of video app, it requires at least three metadata attributes: height, width and duration.
When rtsp connection is established, the following metadata are available from SDP:
- TotalTracks
- MimeType
- Duration
- ChannelCount and SampleRate for audio
- Width and Height for video

> ::: content/media/omx/RtspOmxReader.h
> @@ +68,5 @@
> > +  // DECODING state.
> > +  virtual VideoData* FindStartTime(int64_t& aOutStartTime)
> > +    MOZ_FINAL MOZ_OVERRIDE {
> > +    return nullptr;
> > +  }
> This means mStartTime will be set to 0 in
> MediaDecoderStateMachine::FindStartTime, and mEndTime will not be set. Since
> OnConnected has happened, do you have a start time or end time in the
> metadata? Can you access those in FindStartTime?
Yes, mStartTime will be 0.
But mEndTime was already set as mDuration by the following call path:
RtspMediaResource::OnConnected() ==> MediaDecoder::SetDuration() ==> MediaDecoderStateMachine::SetDuration()

I am not quite sure I answered your questions or not. Please feedback and let me know. :)

Comment 9

[Jason Smith [:jsmith]]

RTSP is not a blocking feature for 1.3 (it's targeted), so I do not see why this bug is a blocker.

Comment 10

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Ethan Tseng [:ethan] from comment #8)

> I am not quite sure I answered your questions or not. Please feedback and
> let me know. :)

I think you answered my concerns :) Please extend the comment in TryLoad to explain that for RTSP the metadata will be obtained through SDP at connection time. Then I'll r+ the patch.

Comment 11

[Ethan Tseng [:ethan]]

Attachment: bug-938512-fix.patch

Just add one line of comment.

Comment 12

[Ethan Tseng [:ethan]]

Comment on attachment 8341470 [details] [diff] [review]
bug-938512-fix.patch

Hi Steve, I just added the comment. :)

Comment 13

[Ethan Tseng [:ethan]]

The result of Try Server:
https://tbpl.mozilla.org/?tree=Try&rev=4fab825cb938

Comment 14

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8341470 [details] [diff] [review]
bug-938512-fix.patch

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

Thanks Ethan! Did you just want feedback on this or a full review?

Comment 15

[Ethan Tseng [:ethan]]

I just want to confirm that I can check in the patch with comments added. :)

Comment 16

[Ethan Tseng [:ethan]]

(In reply to Steve Workman [:sworkman] from comment #14)
> Thanks Ethan! Did you just want feedback on this or a full review?
Oh, I got it. Two days ago, you said "... explain that for RTSP the metadata will be obtained through SDP at connection time. Then I'll r+ the patch.". After I added comment, I tried to use "review?" but somehow it didn't work. That's why I used "feedback?". I just wanted to double check your approval for r+.

Comment 17

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8341470 [details] [diff] [review]
bug-938512-fix.patch

Ah I see. Apologies - I should just have r+d it then. r=me.

Comment 18

[Ethan Tseng [:ethan]]

Attachment: bug-938512-fix.patch

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ff7f259d9bc

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9ff7f259d9bc

Comment 21

[Jason Smith [:jsmith]]

This is a critical event as part of media for a committed feature, so this is a blocker.