1086198 - [RTSP] The unit of property duration of media element is not in seconds

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RTSP, defect)

Description

[Ethan Tseng [:ethan]]

The property "duration" of media element indicates the length of the media in 
seconds, or zero if no media is available.

In the WIP patch of bug 1032111, we create a video element and specify an RTSP
link as its src. After the media is loaded, we obtain the property duration and
find it is in microseconds, not seconds.

/*** JS code snippet ***/
videoElement = document.createElement("video");
source = document.createElement("source");
source.src = aUrl;

videoElement.appendChild(source);
document.body.appendChild(videoElement);
videoElement.play();

// wait for loaded ....
videoDuration = videoElement.duration;  // this is in microseconds!
/*** end of JS code snippet ***/

We compared RTSP with HTTP stream. The latter has the duration in seconds.

Comment 1

[Ethan Tseng [:ethan]]

http://dxr.mozilla.org/mozilla-central/source/content/media/RtspMediaResource.cpp#724

RtspMediaResource::OnConnected() passes the duration in units of microseconds to
MediaDecoder::SetDuration().

Comment 2

[Ethan Tseng [:ethan]]

Attachment: Fix the unit of argument to Decoder::SetDuration()

Comment 3

[Ethan Tseng [:ethan]]

Comment on attachment 8510105 [details] [diff] [review]
Fix the unit of argument to Decoder::SetDuration()

Benjamin, can you review this patch?

Comment 4

[Benjamin Chen [:bechen]]

Comment on attachment 8510105 [details] [diff] [review]
Fix the unit of argument to Decoder::SetDuration()

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

Hi Chris:
Do you know what's the difference between MediaDecoder::SetDuration and MediaDecoder::SetMediaDuration?
In this case, which one we should use?

::: content/media/RtspMediaResource.cpp
@@ +725,1 @@
>    } else {

Should we use USECS_PER_S?

Comment 5

[Ethan Tseng [:ethan]]

Attachment: Fix the unit of argument to Decoder::SetDuration()

Hi Chris,

Could you review this patch?

This is a small but silly bug.
We haven't noticed it until recently because the video control always shows 
the correct duration. However, it is wrong when we retrieve the duration from
a media element containing RTSP stream as its source.

Meanwhile, I don't quite understand the difference between MediaDecoder::
SetDuration() and SetMediaDuration().
Could we invoke the latter so that we can avoid unnecessary unit conversions?

Comment 6

[Ethan Tseng [:ethan]]

Attachment: Fix the unit of argument to Decoder::SetDuration()

Refine code comment.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8510214 [details] [diff] [review]
Fix the unit of argument to Decoder::SetDuration()

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

r=cpearce with change below.

::: content/media/RtspMediaResource.cpp
@@ +721,4 @@
>      // Not live stream.
>      mRealTime = false;
>      mDecoder->SetInfinite(false);
> +    mDecoder->SetDuration(durationUs / USECS_PER_S);

You should probably use:

double(durationUs) / USECS_PER_S

Otherwise the duration may be calculated as (double((int64_t)durationUs) / (int64_t)USECS_PER_S)), which would round to the nearest second, which is not what we want for a double value.

Comment 8

[Ethan Tseng [:ethan]]

Comment on attachment 8510214 [details] [diff] [review]
Fix the unit of argument to Decoder::SetDuration()

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

::: content/media/RtspMediaResource.cpp
@@ +721,4 @@
>      // Not live stream.
>      mRealTime = false;
>      mDecoder->SetInfinite(false);
> +    mDecoder->SetDuration(durationUs / USECS_PER_S);

Right! Thanks for reminding me that.

Comment 9

[Ethan Tseng [:ethan]]

Attachment: Fix the unit of argument to Decoder::SetDuration()

Addressed the issue in the review comment.

Comment 10

[Ethan Tseng [:ethan]]

The result of Treeherder:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d87a0dd29f4f

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/4832e93e168e

Comment 12

[Carsten Book [:Tomcat]]

sorry had to back this out for bustage like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=697519&repo=b2g-inbound

Comment 13

[Ethan Tseng [:ethan]]

Attachment: Fix the unit of argument to Decoder::SetDuration()

Include VideoUtils.h to fix build bustage.

Comment 14

[Ethan Tseng [:ethan]]

Sorry I should run debug build as well.

The result of Treeherder for the updated patch:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d0cbc3150039

Comment 15

[Ethan Tseng [:ethan]]

Hi Carsten,

I requested to check in the updated patch.
Could you help on that? :)

Comment 16

[Ethan Tseng [:ethan]]

Oops! I just found some code paths were changed.
I need to rebase my patch. Cancel the request.

Comment 17

[Ethan Tseng [:ethan]]

Attachment: Fix the unit of argument to Decoder::SetDuration()

Rebase the patch because the directory m-c/content/media was moved to
m-c/dom/media.

Comment 18

[Ethan Tseng [:ethan]]

The result of Treeherder:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=30e6820beaeb

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0910fa2f279a

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0910fa2f279a

Comment 21

[Ethan Tseng [:ethan]]

Comment on attachment 8512503 [details] [diff] [review]
Fix the unit of argument to Decoder::SetDuration()

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None
User impact if declined: Could get incorrect duration of media element.
Testing completed: On m-c
Risk to taking this patch (and alternatives if risky): None
String or UUID changes made by this patch: None

Comment 22

[bhavana bajaj [:bajaj]]

:ethan, from the read of the comments the issue does not look major and this can ride the trains. Please explain the user impact if you consider this to be a ship-blocker which in the kind of issue qualify for uplift/blocking here.

Comment 23

[Ethan Tseng [:ethan]]

(In reply to bhavana bajaj [:bajaj] from comment #22)
> :ethan, from the read of the comments the issue does not look major and this
> can ride the trains. Please explain the user impact if you consider this to
> be a ship-blocker which in the kind of issue qualify for uplift/blocking
> here.
Hi Bhavana,

Right. This issue is not major. Let's put it ride the trains.
Then I'll cancel the uplift requests. Thanks for your time.

Comment 25

[Ethan Tseng [:ethan]]

Attachment: bug-1086198-fix-v2.1

The same fix but rebased for v2.1 (mozilla-b2g34_v2_1).

Previously we decided not to uplift the patch to v2.1 (comment 22 and 23)
because users should not notice this bug.
However, one of our partners found a random duration issue in bug 1112030
recently. If necessary, we can uplift the fix to v2.1.

Comment 26

[Ethan Tseng [:ethan]]

Comment on attachment 8537846 [details] [diff] [review]
bug-1086198-fix-v2.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 1112030
User impact if declined: Incorrect duration is shown in RTSP streaming
Testing completed: On v2.1
Risk to taking this patch (and alternatives if risky): None
String or UUID changes made by this patch: None

Comment 27

[Ethan Tseng [:ethan]]

The result of Treeherder:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac5768332de7

Comment 28

[bhavana bajaj [:bajaj]]

Comment on attachment 8537846 [details] [diff] [review]
bug-1086198-fix-v2.1

Approving given the latest partner issue.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/cc9a90b5d66b

Comment 30

[[e-mail]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #29)
> https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/cc9a90b5d66b

Hi Ryan:

Please help to check this fix already landed in v2.1.

Thanks!

Comment 31

[Ryan VanderMeulen [:RyanVM]]

I'm not QA.