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

RESOLVED FIXED in Firefox 28, Firefox OS v1.3

Status

()

Core
Audio/Video
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ethan, Assigned: ethan)

Tracking

unspecified
1.3 Sprint 6 - 12/6
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:1.3+, firefox28 fixed, b2g-v1.3 fixed)

Details

(Whiteboard: [ft:ril])

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 832105 [details] [diff] [review]
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.
Attachment #832105 - Flags: review?
(Assignee)

Updated

5 years ago
Assignee: nobody → ettseng
Blocks: 928332
blocking-b2g: --- → 1.3?
Target Milestone: --- → 1.3 Sprint 5 - 11/22
(Assignee)

Updated

5 years ago
Attachment #832105 - Flags: review?
Whiteboard: [ft:ril]
Target Milestone: 1.3 Sprint 5 - 11/22 → 1.3 Sprint 6 - 12/6
(Assignee)

Comment 2

5 years ago
*** 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.
blocking-b2g: 1.3? → 1.3+
(Assignee)

Comment 3

5 years ago
*** 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().
Component: General → Video/Audio
Product: Firefox OS → Core
(Assignee)

Comment 4

5 years ago
Created attachment 8338381 [details] [diff] [review]
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.
Attachment #832105 - Attachment is obsolete: true
Attachment #8338381 - Flags: review?(sworkman)
(Assignee)

Comment 5

5 years ago
Created attachment 8338622 [details]
MediaElementStateMachine.png

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

Comment 6

5 years ago
Created attachment 8338625 [details]
MediaElementStateMachine.png
Attachment #8338622 - Attachment is obsolete: true
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?
(Assignee)

Comment 8

5 years ago
(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. :)
RTSP is not a blocking feature for 1.3 (it's targeted), so I do not see why this bug is a blocker.
blocking-b2g: 1.3+ → 1.3?
(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.
(Assignee)

Comment 11

5 years ago
Created attachment 8341470 [details] [diff] [review]
bug-938512-fix.patch

Just add one line of comment.
Attachment #8338381 - Attachment is obsolete: true
Attachment #8338381 - Flags: review?(sworkman)
(Assignee)

Comment 12

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

Hi Steve, I just added the comment. :)
Attachment #8341470 - Flags: feedback?(sworkman)
(Assignee)

Comment 13

5 years ago
The result of Try Server:
https://tbpl.mozilla.org/?tree=Try&rev=4fab825cb938
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?
Attachment #8341470 - Flags: feedback?(sworkman) → feedback+
(Assignee)

Comment 15

5 years ago
I just want to confirm that I can check in the patch with comments added. :)
(Assignee)

Comment 16

5 years ago
(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 on attachment 8341470 [details] [diff] [review]
bug-938512-fix.patch

Ah I see. Apologies - I should just have r+d it then. r=me.
Attachment #8341470 - Flags: review+
(Assignee)

Comment 18

5 years ago
Created attachment 8343494 [details] [diff] [review]
bug-938512-fix.patch
Attachment #8341470 - Attachment is obsolete: true
Attachment #8343494 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9ff7f259d9bc
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
This is a critical event as part of media for a committed feature, so this is a blocker.
blocking-b2g: 1.3? → 1.3+
status-b2g-v1.3: --- → fixed
status-firefox28: --- → fixed
You need to log in before you can comment on or make changes to this bug.