Closed Bug 998239 Opened 6 years ago Closed 6 years ago

[RTSP] Follow up of 877193 to refine RtspTrackBuffer size

Categories

(Firefox OS Graveyard :: RTSP, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
1.4 S6 (25apr)
tracking-b2g backlog

People

(Reporter: ethan, Assigned: ethan)

Details

(Whiteboard: [p=1])

Attachments

(1 file)

Bug 877193 reduces memory usage of RTSP.
Memory allocated for each video track is reduced from 64MB to 4MB.

However, after bug 877193 was landed, Benjamin Chen and I had an offline discussion.
He suggests it's better to lower the maximum size of a slot instead of lowering the number of slots.

The problem of "small-number-of-slots and large-slot-size" is that if RTP packets carrying a lot of small NAL units arrive in a short time, the slots will become full quickly, which might impact RTSP streaming performance.
By contrast, if RTP packets contain large NAL units, each NAL unit will occupy multiple slots according to current design. So it wouldn't be an issue in this case.

In conclusion, it's better to reverse the change to "large-number-of-slots and small-slot-size".
Assignee: nobody → ettseng
Hi Benjamin,
Please help to see if my description and patch are reasonable.
Any comment is welcome.
Flags: needinfo?(bechen)
Rtsp usually has two tracks, one for video, another for audio.
So If we want to reduce the total memory usage to 4MB, should we have a mechanism to divide 4MB between audio/video track? Instead 4MB + 4MB for each.
Flags: needinfo?(bechen)
(In reply to Benjamin Chen [:bechen] from comment #2)
> Rtsp usually has two tracks, one for video, another for audio.
> So If we want to reduce the total memory usage to 4MB, should we have a
> mechanism to divide 4MB between audio/video track? Instead 4MB + 4MB for
> each.
It's not 4MB for audio track.
http://dxr.mozilla.org/mozilla-central/source/content/media/RtspMediaResource.cpp#549
Since the width/height of audio tracks are zero, its size is determined by BUFFER_SLOT_DEFAULT_SIZE.
If we change BUFFER_SLOT_NUM back to 8192, the buffer size of an audio track will be: 8K * 256 = 2MB.

IMHO, if we want to further optimize the memory usage of RTSP, we should analyze the slot usages in real world cases. For now, I think it's fine to allocate 6MB in total. How do you think?
Comment on attachment 8408832 [details] [diff] [review]
Refine RtspTrackBuffer size

Hi Steve,

I apologize.
In https://bugzilla.mozilla.org/show_bug.cgi?id=877193#c6, I replied to you the performance is unaffected without cautious consideration.
Actually that patch has "potential" performance impact (comment 0).

I guess it's better to rectify this issue when it's still fresh in our memory.
Attachment #8408832 - Flags: review?(sworkman)
Status: NEW → ASSIGNED
Comment on attachment 8408832 [details] [diff] [review]
Refine RtspTrackBuffer size

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

:) No problem.
Attachment #8408832 - Flags: review?(sworkman) → review+
(In reply to Ethan Tseng [:ethan] from comment #3)
> IMHO, if we want to further optimize the memory usage of RTSP, we should
> analyze the slot usages in real world cases. For now, I think it's fine to
> allocate 6MB in total. How do you think?

Another way to do this is to use estimates from your tests "in the lab". Start with these and then gather telemetry from the real world.

And then, of course, there could be options to make the size adaptive based on device memory, current free memory etc. But that might be a bit down the line.
Please this into backlog.
blocking-b2g: --- → backlog
(In reply to Steve Workman [:sworkman] from comment #6)
> Another way to do this is to use estimates from your tests "in the lab".
> Start with these and then gather telemetry from the real world.
> And then, of course, there could be options to make the size adaptive based
> on device memory, current free memory etc. But that might be a bit down the
> line.
Good advices.
We can take this into consideration when refactoring RtspTrackBuffer by using MediaCache.
I will keep following up this issue with Benjamin Chen.
The result of TBPL:
https://tbpl.mozilla.org/?tree=Try&rev=a34c803115ab
Keywords: checkin-needed
Whiteboard: [p=1]
Target Milestone: --- → 1.4 S6 (25apr)
https://hg.mozilla.org/mozilla-central/rev/d56bedaa091f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.