Closed Bug 1086198 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox OS Graveyard :: RTSP, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox34 wontfix, firefox35 wontfix, firefox36 fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

RESOLVED FIXED
2.1 S8 (7Nov)
Tracking Status
firefox34 --- wontfix
firefox35 --- wontfix
firefox36 --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: ethan, Assigned: ethan)

References

Details

(Keywords: verifyme, Whiteboard: [p=1])

Attachments

(2 files, 5 obsolete files)

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.
Assignee: nobody → ettseng
Blocks: 1032111
http://dxr.mozilla.org/mozilla-central/source/content/media/RtspMediaResource.cpp#724

RtspMediaResource::OnConnected() passes the duration in units of microseconds to
MediaDecoder::SetDuration().
Summary: [RTSP] Unit of property duration of media element is not in seconds → [RTSP] The unit of property duration of media element is not in seconds
Comment on attachment 8510105 [details] [diff] [review]
Fix the unit of argument to Decoder::SetDuration()

Benjamin, can you review this patch?
Attachment #8510105 - Flags: review?(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?
Attachment #8510105 - Flags: review?(bechen) → review?(cpearce)
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?
Attachment #8510105 - Attachment is obsolete: true
Attachment #8510105 - Flags: review?(cpearce)
Attachment #8510126 - Flags: review?(cpearce)
Refine code comment.
Attachment #8510126 - Attachment is obsolete: true
Attachment #8510126 - Flags: review?(cpearce)
Attachment #8510214 - Flags: review?(cpearce)
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.
Attachment #8510214 - Flags: review?(cpearce) → review+
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.
Addressed the issue in the review comment.
Attachment #8510214 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Whiteboard: [p=1]
Target Milestone: --- → 2.1 S7 (24Oct)
Include VideoUtils.h to fix build bustage.
Attachment #8510845 - Attachment is obsolete: true
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
Keywords: checkin-needed
Hi Carsten,

I requested to check in the updated patch.
Could you help on that? :)
Flags: needinfo?(cbook)
Oops! I just found some code paths were changed.
I need to rebase my patch. Cancel the request.
Keywords: checkin-needed
Rebase the patch because the directory m-c/content/media was moved to
m-c/dom/media.
Attachment #8511073 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/0910fa2f279a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: 2.1 S7 (24Oct) → 2.1 S8 (7Nov)
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
Attachment #8512503 - Flags: approval-mozilla-b2g34?
Attachment #8512503 - Flags: approval-mozilla-b2g32?
Flags: needinfo?(cbook)
: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.
Flags: needinfo?(ettseng)
(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.
Flags: needinfo?(ettseng)
Attachment #8512503 - Flags: approval-mozilla-b2g34?
Attachment #8512503 - Flags: approval-mozilla-b2g32?
Duplicate of this bug: 1112030
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.
Attachment #8537846 - Flags: review+
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
Attachment #8537846 - Flags: approval-mozilla-b2g34?
Comment on attachment 8537846 [details] [diff] [review]
bug-1086198-fix-v2.1

Approving given the latest partner issue.
Attachment #8537846 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
(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!
Flags: needinfo?(ryanvm)
I'm not QA.
Flags: needinfo?(ryanvm)
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.