Closed
Bug 726889
Opened 13 years ago
Closed 13 years ago
rename nsMediaStream
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: roc, Assigned: roc)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
nsMediaStream conflicts with "MediaStream" DOM APIs. Let's rename it to mozilla::MediaResource/ChannelMediaResource/FileMediaResource.
Assignee | ||
Comment 1•13 years ago
|
||
Attachment #596904 -
Flags: review?(cpearce)
Comment 2•13 years ago
|
||
Comment on attachment 596904 [details] [diff] [review]
fix
Review of attachment 596904 [details] [diff] [review]:
-----------------------------------------------------------------
There's some variables called "stream" which should be "resource", e.g.: |MediaResource* stream = mDecoder->GetResource();|. May as well make them all "resource" for consistency.
::: content/media/nsMediaStream.cpp
@@ +361,5 @@
> }
>
> struct CopySegmentClosure {
> nsCOMPtr<nsIPrincipal> mPrincipal;
> + ChannelMediaResource* mStream;
Do you want to rename this to mResource for consistency? Maybe there's others too?
@@ +537,4 @@
> {
> NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
>
> + ChannelMediaResource* stream = new ChannelMediaResource(aDecoder, nsnull, mURI);
s/stream/resource/ ?
::: content/media/nsMediaStream.h
@@ +409,5 @@
> public nsIInterfaceRequestor,
> public nsIChannelEventSink
> {
> public:
> + Listener(ChannelMediaResource* aStream) : mStream(aStream) {}
s/mStream/mResource/ ?
::: content/media/nsBuiltinDecoderStateMachine.cpp
@@ +1999,5 @@
> }
>
> // Check to see if we don't have enough data to play up to the next frame.
> // If we don't, switch to buffering mode.
> + MediaResource* stream = mDecoder->GetResource();
s/stream/resource/ ? A couple more of them below too.
::: content/media/nsMediaDecoder.h
@@ +143,5 @@
> // point of the first frame of data.
> // aStream is the media stream to use. Ownership of aStream passes to
> // the decoder, even if Load returns an error.
> // This is called at most once per decoder, after Init().
> + virtual nsresult Load(MediaResource* aStream,
s/aStream/aResource/
::: content/media/ogg/nsOggReader.cpp
@@ +81,2 @@
> static PageSyncResult
> +PageSync(MediaResource* aStream,
aResource
@@ +1074,2 @@
> static PageSyncResult
> +PageSync(MediaResource* aStream,
aResource
@@ +1471,5 @@
> startTime = nsTheoraState::Time(&mTheoraInfo, granulepos);
> NS_ASSERTION(startTime > 0, "Must have positive start time");
> }
> else if (IsKnownStream(serial)) {
> + // stream is not the theora or vorbis stream we're playing,
s/stream/Stream/. This is not the stream you're looking for. ;)
::: content/media/raw/nsRawReader.cpp
@@ +76,2 @@
>
> + if (!ReadFromStream(resource, reinterpret_cast<PRUint8*>(&mMetadata),
ReadFromResource
@@ +202,5 @@
> !(header.packetID == 0xFF && header.codecID == RAW_ID /* "YUV" */)) {
> return false;
> }
>
> + if (!ReadFromStream(resource, buffer, length)) {
ReadFromResource
::: content/media/webm/nsWebMReader.cpp
@@ +450,5 @@
> }
> if (tstamp_frames > decoded_frames) {
> #ifdef DEBUG
> PRInt64 usecs = 0;
> + LOG(PR_LOG_DEBUG, ("WebMReader detected gap of %lld, %lld frames, in audio resource\n",
s/resource/stream/. This is not the stream you're looking for.
Attachment #596904 -
Flags: review?(cpearce) → review-
Assignee | ||
Comment 3•13 years ago
|
||
All fixed.
Attachment #596904 -
Attachment is obsolete: true
Attachment #596908 -
Flags: review?(cpearce)
Assignee | ||
Comment 4•13 years ago
|
||
This might conflict with Blob support that Jonas was asking me about.
Comment 5•13 years ago
|
||
Comment on attachment 596908 [details] [diff] [review]
fix v2
Review of attachment 596908 [details] [diff] [review]:
-----------------------------------------------------------------
r+ with three nits picked. Looks good.
::: content/media/nsMediaCache.h
@@ +191,5 @@
> * to try to avoid requesting seeks on such streams.
> *
> * nsMediaCache has a single internal monitor for all synchronization.
> * This is treated as the lowest level monitor in the media code. So,
> + * we must not acquire any nsMediaDecoder locks or MediaStream locks
Did you mean "MediaResource locks"?
@@ +359,5 @@
> // Returns true when all streams for this resource are suspended or their
> // channel has ended.
> // If aActiveStream is non-null, fills it with a pointer to a stream
> // for this resource that is not suspended or ended.
> + bool AreAllStreamsForResourceSuspended(MediaResource** aActiveStream);
aActiveResource, update the comment too.
::: content/media/nsMediaDecoder.h
@@ +143,5 @@
> // point of the first frame of data.
> // aStream is the media stream to use. Ownership of aStream passes to
> // the decoder, even if Load returns an error.
> // This is called at most once per decoder, after Init().
> + virtual nsresult Load(MediaResource* aStream,
aResource, update the comment.
Attachment #596908 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 6•13 years ago
|
||
Comment 7•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in
before you can comment on or make changes to this bug.
Description
•