Last Comment Bug 726889 - rename nsMediaStream
: rename nsMediaStream
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla13
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
Depends on:
Blocks: 664918
  Show dependency treegraph
 
Reported: 2012-02-13 18:07 PST by Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
Modified: 2012-02-15 09:16 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (130.57 KB, patch)
2012-02-13 19:40 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
cpearce: review-
Details | Diff | Splinter Review
fix v2 (135.72 KB, patch)
2012-02-13 20:29 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
cpearce: review+
Details | Diff | Splinter Review

Description Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-13 18:07:51 PST
nsMediaStream conflicts with "MediaStream" DOM APIs. Let's rename it to mozilla::MediaResource/ChannelMediaResource/FileMediaResource.
Comment 1 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-13 19:40:33 PST
Created attachment 596904 [details] [diff] [review]
fix
Comment 2 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-02-13 20:05:34 PST
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.
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-13 20:29:47 PST
Created attachment 596908 [details] [diff] [review]
fix v2

All fixed.
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-13 20:31:13 PST
This might conflict with Blob support that Jonas was asking me about.
Comment 5 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-02-13 20:50:33 PST
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.
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-14 21:15:03 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/eff64129a6f2
Comment 7 Marco Bonardo [::mak] 2012-02-15 09:16:20 PST
https://hg.mozilla.org/mozilla-central/rev/eff64129a6f2

Note You need to log in before you can comment on or make changes to this bug.