Closed
Bug 998239
Opened 10 years ago
Closed 10 years ago
[RTSP] Follow up of 877193 to refine RtspTrackBuffer size
Categories
(Firefox OS Graveyard :: RTSP, defect)
Tracking
(tracking-b2g:backlog)
People
(Reporter: ethan, Assigned: ethan)
Details
(Whiteboard: [p=1])
Attachments
(1 file)
1.11 KB,
patch
|
sworkman
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•10 years ago
|
Assignee: nobody → ettseng
Assignee | ||
Comment 1•10 years ago
|
||
Hi Benjamin, Please help to see if my description and patch are reasonable. Any comment is welcome.
Flags: needinfo?(bechen)
Comment 2•10 years ago
|
||
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)
Assignee | ||
Comment 3•10 years ago
|
||
(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?
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Comment 5•10 years ago
|
||
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+
Comment 6•10 years ago
|
||
(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.
Assignee | ||
Comment 8•10 years ago
|
||
(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.
Assignee | ||
Comment 9•10 years ago
|
||
The result of TBPL: https://tbpl.mozilla.org/?tree=Try&rev=a34c803115ab
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/d56bedaa091f
Keywords: checkin-needed
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d56bedaa091f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•