If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Improve performance of Stagefright internal http server

RESOLVED WONTFIX

Status

()

Core
Audio/Video: Playback
RESOLVED WONTFIX
4 years ago
a year ago

People

(Reporter: cajbir, Unassigned, Mentored)

Tracking

Trunk
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=c++])

(Reporter)

Description

4 years ago
The stagefright internal http server introduced in bug 860599 is not optimal for performance. It uses a blocking/threaded structure with unneeded copying of data. This should be improved as recommended by Honza Bambas un bug 860599 comment 34.
(Reporter)

Updated

4 years ago
Whiteboard: [lang=c++][mentor=doublec]
(Reporter)

Updated

4 years ago
Depends on: 860599

Comment 1

4 years ago
I would be happy to take a look if no one has fixed this yet.  Just let me know what files I'll need to look at and I'll get started.
(Reporter)

Comment 2

4 years ago
That would be great! The existing http server code is in:

content/media/plugins/MediaResourceServer.h
content/media/plugins/MediaResourceServer.cpp

The server is instantiated in:

content/media/plugins/MediaPluginHost.cpp

It is accessed from:

media/omx/OmxPlugin.cpp (OmxDecoder::Init)

The main code will bein the MediaResourceServer files.

Comment 3

4 years ago
Alright, I've taken a look at the bug you linked in your first comment.  I'm completely new to this codebase so I don't really know what a lot of these classes do yet.  From what I gathered from Honza's post, the current implementation is inefficient because of how many times it unnecessarily copies code.  It looks like he has identified the copy to a pipe here http://hg.mozilla.org/mozilla-central/annotate/dbd7d55d64ff/netwerk/base/src/nsSocketTransport2.cpp#l1772 in the routine for opening an input stream (when OPEN_BLOCKING is passed as a parameter) as a copy that could be eliminated.  It looks to me like the code for opening an unbuffered input stream in the same function does avoid this copy because it doesn't use the pipe, however, wouldn't we run into concurrency issues with that approach or am I completely confused?  

I'm also quite confused about what this suggestion by Honza means: "You may implement nsIInputStream that wraps the MediaResource.  This way you can use stream copier (via WriteSegments to the socket output stream)."

Comment 4

4 years ago
nsIInputStream is an interface defined here: http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsIInputStream.idl . It allows reading from some arbitrary source in a controlled, efficient way. There are various examples of classes that implement it, usually named nsSomethingStream: http://mxr.mozilla.org/mozilla-central/source/xpcom/io/

Comment 5

4 years ago
Ok first of all sorry I disappeared for a few days, between classes, job interviews and a midterm I was just absolutely swamped.

Anyways, I've read through those links Josh posted and several other interfaces involved and I think I'm starting to get at least some idea of what's going on here.  I looks like what I need to do to start is make a new class that implements nsIInputStream, and wraps the MediaResource class so that we have a clean, controlled way of reading from the MediaResource and can use stream copier?

Updated

3 years ago
Mentor: cajbir.bugzilla@cd.pn
Whiteboard: [lang=c++][mentor=doublec] → [lang=c++]
Component: Audio/Video → Audio/Video: Playback

Comment 6

a year ago
This sounds like something I would like to work on - of course, provided that it is still needed?
Flags: needinfo?(cajbir.bugzilla)

Comment 7

a year ago
I'm not sure - Josh, do you have any insight here?
Flags: needinfo?(josh)

Comment 8

a year ago
Certainly something has changed since comment 2 because I am no longer able to locate the code.
Looks like the files form comment 2 are in dom/media/android/ now: https://dxr.mozilla.org/mozilla-central/source/dom/media/android/AndroidMediaResourceServer.h
Flags: needinfo?(josh)

Comment 10

a year ago
Thanks Josh. Looking briefly at the code, I assume this is still relevant?
Maybe Anthony can answer that question, since he oversees the media team.
Flags: needinfo?(cajbir.bugzilla) → needinfo?(ajones)
Status: NEW → RESOLVED
Last Resolved: a year ago
Flags: needinfo?(ajones)
Resolution: --- → WONTFIX
This only affects older android versions. We're not going try to fix anything here.

Comment 13

a year ago
Fair enough, I will find something else.
You need to log in before you can comment on or make changes to this bug.