726889 - rename nsMediaStream

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

nsMediaStream conflicts with "MediaStream" DOM APIs. Let's rename it to mozilla::MediaResource/ChannelMediaResource/FileMediaResource.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

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) (email my personal email if necessary)]

Attachment: fix v2

All fixed.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This might conflict with Blob support that Jonas was asking me about.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

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) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eff64129a6f2

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/eff64129a6f2