Closed Bug 1015985 Opened 6 years ago Closed 6 years ago

Have a CanSeek() function on MediaDecoderReader, then Readers don't need to call out to the MediaDecoder to report that they can seek in ReadMetadata().

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: s.pheric, Assigned: s.pheric)

Details

Attachments

(1 file, 12 obsolete files)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140428193838
Component: Untriaged → Video/Audio
Product: Firefox → Core
Version: 29 Branch → Trunk
Assignee: nobody → eric.phan
Attached file bug-1015985-fix (obsolete) —
Attachment #8432367 - Flags: review+
Attachment #8432367 - Attachment is obsolete: true
Attachment #8432367 - Attachment is patch: false
Attached patch bug-1015985-fix (obsolete) — Splinter Review
Attachment #8432424 - Flags: review+
Comment on attachment 8432424 [details] [diff] [review]
bug-1015985-fix

Review of attachment 8432424 [details] [diff] [review]:
-----------------------------------------------------------------

Chris, does my comment in DirectShowReader.cpp makes sense?

::: content/media/MediaDecoderReader.h
@@ +162,5 @@
>    nsresult DecodeToTarget(int64_t aTarget);
>  
>    MediaInfo GetMediaInfo() { return mInfo; }
>  
> + 

nit: trailing space.

::: content/media/directshow/DirectShowReader.cpp
@@ +212,5 @@
>    DWORD seekCaps = 0;
>    hr = mMediaSeeking->GetCapabilities(&seekCaps);
>    bool canSeek = ((AM_SEEKING_CanSeekAbsolute & seekCaps) == AM_SEEKING_CanSeekAbsolute);
>    if (!canSeek) {
>      mDecoder->SetMediaSeekable(false);

The goal of this bug is to be able to remove the state from the MediaDecoder, and only have it in this class. You should remove m{Transport,Media}Seekable from the MediaDecoder class, and fix the current uses by calling into the MediaDecoderReader directly.

Then, I think you can do the same for the MediaDecoderStateMachine, that would directly ask the MediaDecoderReader. Extra care should be taken regarding on which threads the data is accesed (StateMachine thread, DecoderThread, MainThread).

I think what Chris had in mind was to have the "seekable state" only in one place instead of having multiple copies of the same state that were hard to synchronize.

@@ +213,5 @@
>    hr = mMediaSeeking->GetCapabilities(&seekCaps);
>    bool canSeek = ((AM_SEEKING_CanSeekAbsolute & seekCaps) == AM_SEEKING_CanSeekAbsolute);
>    if (!canSeek) {
>      mDecoder->SetMediaSeekable(false);
> +    this->mIsMediaSeekable = false;

In Gecko, we prefer to omit redundant `this->`, because members are prefixed with the letter "m", so it is already clear that we are dealing with an instance member. Please change all the occurrences in this patch.
Attachment #8432424 - Flags: review+ → feedback?(cpearce)
Attached patch bug-1015985-fix (obsolete) — Splinter Review
Attachment #8433433 - Flags: review?(cpearce)
Comment on attachment 8432424 [details] [diff] [review]
bug-1015985-fix

Review of attachment 8432424 [details] [diff] [review]:
-----------------------------------------------------------------

There's no need to remove MediaDecoder knowing that the transport/media is seekable at this point.

We want to work towards removing the need for MediaDecoderReader to know anything about MediaDecoder. We'll do that in stages.

So to start with, please remove the callbacks from the MediaDecoderReader subclasses to MediaDecoder::SetMediaSeekable(), and add functions  MediaDecoderReader::IsMediaSeekable() and IsTransportSeekable(). MediaDecoderStateMachine::DecodeMetadata() should call these after calling MediaDecoderReader::ReadMetadata(), and then call MediaDecoder::SetMediaSeekable()/SetTransportSeekable() to store the result.

::: content/media/MediaDecoderReader.h
@@ +165,5 @@
>  
> + 
> +  // Indicates if the reader associated to the decoder is seekable,
> +  // for the transport and the media
> +  bool CanSeekMedia();

virtual bool IsMediaSeekable() = 0;
virtual bool IsTransportSeekable() = 0;

@@ +181,5 @@
>    // directly, because they have a number of channel higher than
>    // what we support.
>    bool mIgnoreAudioOutputFormat;
> +
> +  bool mIsMediaSeekable;

Don't add these members. Instead, copy the code from the ReadMetadata() implementations into the IsMediaSeekable() implementation to that determine whether we can seek.

The idea being we should not store what we can calculate. If we store data, it can get out of date. If we calculate it every time, it's less likely to be out of date.
Attachment #8432424 - Flags: feedback?(cpearce)
Comment on attachment 8433433 [details] [diff] [review]
bug-1015985-fix

Review of attachment 8433433 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/src/HTMLMediaElement.cpp
@@ +2515,5 @@
>  
>    double duration = aOriginal->GetDuration();
>    if (duration >= 0) {
>      decoder->SetDuration(duration);
> +    decoder->GetStateMachine()->GetReader()->SetTransportSeekable(aOriginal->GetStateMachine()->GetReader()->CanSeekTransport());

I do not want to expose the state machine to the HTMLMediaElement.

Software components gets easier to understand the less they do, and the fewer entry points they have.

I don't remember why we need to do this. Do mochitests fail if we don't do this? We should recalculate these once the cloned media has finished loading metadata, and I don't understand how it could calculate different values.

@@ +2516,5 @@
>    double duration = aOriginal->GetDuration();
>    if (duration >= 0) {
>      decoder->SetDuration(duration);
> +    decoder->GetStateMachine()->GetReader()->SetTransportSeekable(aOriginal->GetStateMachine()->GetReader()->CanSeekTransport());
> +    decoder->GetStateMachine()->GetReader()->SetMediaSeekable(aOriginal->GetStateMachine()->GetReader()->CanSeekMedia());

Note in your MediaDecoderReader::SetMediaSeekable() implementation you assert that it's only called on the decode thread, whereas here you call it on the main thread. I'm guessing you didn't try running the `./mach mochitest-plain content/media/`? I bet it'll assert if you do.

Ideally, we want MediaDecoderReader to be called on as few threads as possible. So don't store seekable state on the reader, store it on the MediaDecoder, which is supposed to be the main thread cache of the media stack's off main thread state.

::: content/media/MediaDecoderReader.h
@@ +184,5 @@
>    // what we support.
>    bool mIgnoreAudioOutputFormat;
> +
> +  // Indicates whether the media and transport are seekable.
> +  bool mMediaSeekable;

The reason why I don't want to require ReadMetadata() to set a class member mMediaSeekable is that it's very easy to forget to do this. Whereas the compiler will not let you compile if you forget to implement a virtual IsMediaSeekable()=0; function.

::: content/media/MediaDecoderStateMachine.h
@@ +139,5 @@
>      AssertCurrentThreadInMonitor();
>      return mState;
>    }
>  
> +  MediaDecoderReader* GetReader() {

Do not expose the reader. We want to reduce the number of ways through which the various parts of our system can access each other, not increase them.
Attachment #8433433 - Flags: review?(cpearce) → review-
(In reply to Chris Pearce (:cpearce) from comment #6)

Thank you cpearce. All your comments were taken into account. Thanks to your explanations, now we understand much more what the problem is about. In fact, we might have misunderstood the comment #3

(From comment #3)
> Comment on attachment 8432424 [details] [diff] [review]
> bug-1015985-fix
> 
> Review of attachment 8432424 [details] [diff] [review]:
> 
> The goal of this bug is to be able to remove the state from the
> MediaDecoder, and only have it in this class. You should remove
> m{Transport,Media}Seekable from the MediaDecoder class, and fix the current
> uses by calling into the MediaDecoderReader directly.


which states that some attributes should be removed from MediaDecoder, thus we've stored them into MediaDecoderReader.
Attachment #8433433 - Attachment is obsolete: true
Attached patch bug-1015985-fix (obsolete) — Splinter Review
Attachment #8433922 - Flags: review?(cpearce)
Comment on attachment 8433922 [details] [diff] [review]
bug-1015985-fix

Review of attachment 8433922 [details] [diff] [review]:
-----------------------------------------------------------------

That's looking much better. I'm concerned that we have a lot of IsTransportSeekable() methods all returning true. Seems like we should be determining that elsewhere (like on the MediaResource). It's late here, so I'll look into this more tomorrow (so I didn't r+/- yet), or you're welcome to try to figure out if we can rely on MediaResource::IsTransportSeekable() instead here.

::: content/media/MediaDecoderStateMachine.cpp
@@ +1802,5 @@
>    {
>      ReentrantMonitorAutoExit exitMon(mDecoder->GetReentrantMonitor());
>      res = mReader->ReadMetadata(&info, &tags);
> +    {
> +      ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());

You don't need to create a new scope here to take the monitor; the monitor will taken once you leave the scope in which the above ReentrantMonitorAutoExit is declared.

So move the SetMediaSeekable/SetTransportSeekable calls outside of this scope, down at line 1818.
(In reply to Chris Pearce (:cpearce) from comment #9)
> Comment on attachment 8433922 [details] [diff] [review]
> bug-1015985-fix
> 
> Review of attachment 8433922 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> That's looking much better. I'm concerned that we have a lot of
> IsTransportSeekable() methods all returning true. Seems like we should be
> determining that elsewhere (like on the MediaResource). It's late here, so
> I'll look into this more tomorrow (so I didn't r+/- yet), or you're welcome
> to try to figure out if we can rely on MediaResource::IsTransportSeekable()
> instead here.

I agree that having all these IsTransportSeekable() returning true does not seem good.

All overrides of MediaResource::IsTransportSeekable() return true (http://dxr.mozilla.org/mozilla-central/search?q=%2Boverrides%3Amozilla%3A%3AMediaResource%3A%3AIsTransportSeekable%28%29) but one ChannelMediaResource. 

But I am wondering, there are some subclasses like AppleMP3Reader where there is no compilation issue if i don't override the virtual method MediaDecoderReader::IsMediaSeekable() or MediaDecoderReader::IsTransportSeekable().

What is the relation between MediaResource::IsTransportSeekable() and MediaDecoderReader::IsTransportSeekable() ?
(In reply to Chris Pearce (:cpearce) from comment #9)
> Comment on attachment 8433922 [details] [diff] [review]
> bug-1015985-fix
> 
> Review of attachment 8433922 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> That's looking much better. I'm concerned that we have a lot of
> IsTransportSeekable() methods all returning true. Seems like we should be
> determining that elsewhere (like on the MediaResource). It's late here, so
> I'll look into this more tomorrow (so I didn't r+/- yet), or you're welcome
> to try to figure out if we can rely on MediaResource::IsTransportSeekable()
> instead here.

I have checked with MediaResource::IsTransportSeekable() and it don't seem that we can rely on that, there are some tests that fail. So either I haven't done it correctly, or this is not a good way to solve the issue.

I have thought about an other solution, instead of using bool MediaDecoderReader::IsTransportSeekable().
We could use int MediaDecoderReader::IsTransportSeekable(), which would return 0 for false, 1 for true and 3 for don't take this value. Because in ReadMetadata, there are many cases when TransportSeekable is not set. And always forcing "mDecoder->SetTransportSeekable(mReader->IsTransportSeekable())" in MediaDecoderStateMachine::DecodeMetadata should not be correct, in particular when mReader->IsTransportSeekable() returns true. So with int MediaDecoderReader::IsTransportSeekable() we could avoid modifying mDecoder members when there is no need.
Flags: needinfo?(cpearce)
(In reply to Eric Phan from comment #11)
> (In reply to Chris Pearce (:cpearce) from comment #9)
> > Comment on attachment 8433922 [details] [diff] [review]
> > bug-1015985-fix
> > 
> > Review of attachment 8433922 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > That's looking much better. I'm concerned that we have a lot of
> > IsTransportSeekable() methods all returning true. Seems like we should be
> > determining that elsewhere (like on the MediaResource). It's late here, so
> > I'll look into this more tomorrow (so I didn't r+/- yet), or you're welcome
> > to try to figure out if we can rely on MediaResource::IsTransportSeekable()
> > instead here.
> 
> I have checked with MediaResource::IsTransportSeekable() and it don't seem
> that we can rely on that, there are some tests that fail.

Which tests? A number of our tests are (unfortunately) not 100% reliable.

> So either I
> haven't done it correctly, or this is not a good way to solve the issue.

Can I see the patch?


> I have thought about an other solution, instead of using bool
> MediaDecoderReader::IsTransportSeekable().
> We could use int MediaDecoderReader::IsTransportSeekable(), which would
> return 0 for false, 1 for true and 3 for don't take this value. 

The problem with tri-state, is that often (but not always) in practice you end up with the "maybe" case being treated as either the "true" or "false" state. So you may as well just have a bi-state.

Can we just replaces all instances of mDecoder->IsTransportSeekable() with  mDecoder->GetResource()->IsTransportSeekable()?
Flags: needinfo?(cpearce)
(In reply to Chris Pearce (:cpearce) from comment #12)
> 
> Which tests? A number of our tests are (unfortunately) not 100% reliable.

the tests from ./mach mochitest-plain content/media

> > So either I
> > haven't done it correctly, or this is not a good way to solve the issue.
> 
> Can I see the patch?
> 

Sorry, i forgot to attach the patch. It seems that it fails for the test : content/media/test/test_chaining.html (http://dxr.mozilla.org/mozilla-central/source/content/media/test/test_chaining.html). And i believe it's because the value of mMediaSeekable there (http://dxr.mozilla.org/mozilla-central/source/content/media/MediaDecoderStateMachine.cpp#1833) should be false instead of true.

> Can we just replaces all instances of mDecoder->IsTransportSeekable() with 
> mDecoder->GetResource()->IsTransportSeekable()?

That's what I have tried first. I am attaching the patch name 'bug-1015985-resource'
Attached patch bug-1015985-resource (obsolete) — Splinter Review
Here the patch with mDecoder->GetResource()->IsTransportSeekable()
Attachment #8435404 - Flags: review?(cpearce)
Attachment #8433922 - Attachment is obsolete: true
Attachment #8433922 - Flags: review?(cpearce)
Attached patch bug-1015985-tri-state (obsolete) — Splinter Review
I agree that tri-state is not the ideal solution.
However, ReadMetadata does not always set all seekable values, it depends of the decoder used. And as i don't want to delete the decoder seekable members if ReadMetadata does not set them, i would say that a tri-state solution could be a compromise
Attachment #8435409 - Flags: review?(cpearce)
Comment on attachment 8435409 [details] [diff] [review]
bug-1015985-tri-state

Review of attachment 8435409 [details] [diff] [review]:
-----------------------------------------------------------------

What if you have

bool
MediaDecoder::IsTransportSeekable() {
  return GetResource()->IsTransportSeekable();
}

?

Paul Adenot is the expert in Ogg chaining, so he's a good person to ask for help in debugging that failed test.

::: content/media/MediaDecoderStateMachine.cpp
@@ +1810,5 @@
> +    if (seek != 3) {
> +      mDecoder->SetMediaSeekable(seek);
> +    }
> +    seek = mReader->IsTransportSeekable();
> +    if (seek != 3) {

You comment for IsTransportSeekable says:

// Returns 0 for false, 1 for true and 3 for invalid value

MediaDecoder::mTransportSeekable is initialized by MediaDecoder's constructor to true:
http://mxr.mozilla.org/mozilla-central/source/content/media/MediaDecoder.cpp#418

So the logic here is equivalent to:
if (seek == 0) {
  mDecoder->SetTransportSeekable(false);
} else {
  mDecoder->SetTransportSeekable(true);
}

So your tri-state is effectively bi-state.

::: content/media/ogg/OggReader.cpp
@@ +389,5 @@
>  
> +int
> +OggReader::IsTransportSeekable()
> +{
> +  MediaResource* resource = mDecoder->GetResource();

I think in this situation we should be having IsMediaSeekable() returning false. Then we might be able to avoid needing to track IsTransportSeekable() in the *Readers.
Attachment #8435409 - Flags: review?(cpearce) → review-
Comment on attachment 8435404 [details] [diff] [review]
bug-1015985-resource

Review of attachment 8435404 [details] [diff] [review]:
-----------------------------------------------------------------

This is the same patch as the previous?
Attachment #8435404 - Flags: review?(cpearce)
And sorry for the slow review, I'm pretty busy at the moment.
Attached file bug-1015985-fix (obsolete) —
Attachment #8437707 - Flags: review?(cpearce)
Attachment #8435404 - Attachment is obsolete: true
Attachment #8435409 - Attachment is obsolete: true
Comment on attachment 8437707 [details]
bug-1015985-fix

Review of attachment 8437707 [details]:
-----------------------------------------------------------------

You didn't address the review comments I made previously.

You also didn't ensure that your code compiled on all platforms (it won't).

Please address the comment I made last time, fix the compile errors, then get someone (Paul I'd assume) to push the patch to TryServer to determine whether the change we're making here breaks anything.

You can also use TryServer to test whether your patch builds on all platforms too.

::: content/media/MediaDecoderReader.h
@@ +168,5 @@
>  
> +  // Indicates if the media and transport are seekable.
> +  // ReadMetada should be called before calling theses methods.
> +  virtual bool IsMediaSeekable() = 0;
> +  virtual bool IsTransportSeekable() = 0;

Remove MediaDecoderReader::IsTransportSeekable() and all overrides.

::: content/media/MediaDecoderStateMachine.cpp
@@ +1806,5 @@
> +
> +  // affect values only if ReadMetadata succeeds
> +  if (NS_SUCCEEDED(res)) {
> +    mDecoder->SetMediaSeekable(mReader->IsMediaSeekable());
> +    mDecoder->SetTransportSeekable(mReader->IsTransportSeekable());

You didn't address my previous review comments.

Don't call mDecoder->SetTransportSeekable(mReader->IsTransportSeekable());

Instead, chnage MediaDecoder::IsTransportSeekable to return GetResource()->IsTransportSeekable();

::: content/media/directshow/DirectShowReader.cpp
@@ +228,5 @@
>    return NS_OK;
>  }
>  
> +bool
> +DirectShowReader::IsMediaSeekable() MOZ_OVERRIDE

Having MOZ_OVERRIDE here will cause a compile error, and below too.

::: content/media/fmp4/MP4Reader.cpp
@@ +250,5 @@
>    return NS_OK;
>  }
>  
>  bool
> +MP4Reader::IsMediaSeekable() MOZ_OVERRIDE

Having MOZ_OVERRIDE here will cause a compile error, and below too.

::: content/media/gstreamer/GStreamerReader.cpp
@@ +463,5 @@
>    return NS_OK;
>  }
>  
> +bool
> +GStreamerReader::IsMediaSeekable() MOZ_OVERRIDE

Having MOZ_OVERRIDE here will cause a compile error, and below too.

::: content/media/ogg/OggReader.cpp
@@ +387,5 @@
>    return NS_OK;
>  }
>  
> +bool
> +OggReader::IsTransportSeekable()

When you delete the MediaDecoderReader::IsTransportSeekable() overrides, try deleting this too. The IsMediaSeekable() below looks like it'll probably work, but you'll need a green Try run to be sure.

::: content/media/wmf/WMFReader.cpp
@@ +550,5 @@
>    return NS_OK;
>  }
>  
>  bool
> +WMFReader::IsMediaSeekable() MOZ_OVERRIDE

Having MOZ_OVERRIDE here will cause a compile error, and below too.

You must have been building on MacOSX or doing a FirefoxOS build. The build would have failed on every other platform AFAICT.
Attachment #8437707 - Flags: review?(cpearce) → review-
Attached file bug-1015985-fix (obsolete) —
Temporary patch for Try tests.
Attachment #8438475 - Attachment is obsolete: true
Attachment #8438475 - Attachment is patch: false
Attachment #8437707 - Attachment is obsolete: true
Attachment #8437707 - Attachment is patch: false
Attachment #8432424 - Attachment is obsolete: true
Attached patch bug-1015985-fix (obsolete) — Splinter Review
Temporary patch for try tests.
Comment on attachment 8439013 [details] [diff] [review]
bug-1015985-fix

Review of attachment 8439013 [details] [diff] [review]:
-----------------------------------------------------------------

Looking better, this is heading in the right direction.
Attachment #8439013 - Flags: feedback+
Attached patch bug-1015985-fix (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=9d68468a7ba0
Attachment #8439013 - Attachment is obsolete: true
Attachment #8439165 - Flags: review?(cpearce)
Attached patch bug-1015985-fix (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=9d68468a7ba0
Attachment #8439165 - Attachment is obsolete: true
Attachment #8439165 - Flags: review?(cpearce)
Attachment #8439198 - Flags: review?(cpearce)
Comment on attachment 8439198 [details] [diff] [review]
bug-1015985-fix

Review of attachment 8439198 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there. It's looking much better, but I think we can improve things little further.

::: content/media/MediaDecoderReader.h
@@ +92,5 @@
>    // Read header data for all bitstreams in the file. Fills aInfo with
>    // the data required to present the media, and optionally fills *aTags
>    // with tag metadata from the file.
>    // Returns NS_OK on success, or NS_ERROR_FAILURE on failure.
> +  // After calling ReadMetadata, decoder member mMediaSeekable should be

mMediaSeekable is a private member of a separate object, so it doesn't make much sense to talk about it here in this comment.

Just delete this extra comment you added.

::: content/media/MediaDecoderStateMachine.cpp
@@ +2153,5 @@
>      ReentrantMonitorAutoExit exitMon(mDecoder->GetReentrantMonitor());
>      res = mReader->ReadMetadata(&info, getter_Transfers(mMetadataTags));
>    }
> +
> +  // affect values only if ReadMetadata succeeds

// Affect values only if ReadMetadata succeeds.

Also, move this down to after the following block (the one that calls StartWaitForResources()). We don't need to initialize until after we've past that point.

::: content/media/MediaDecoderStateMachine.h
@@ +284,5 @@
>      AssertCurrentThreadInMonitor();
>      return mEndTime;
>    }
>  
>    bool IsTransportSeekable() {

I can't find anything that calls this function, so please delete it.

@@ +291,5 @@
>    }
>  
>    bool IsMediaSeekable() {
>      AssertCurrentThreadInMonitor();
>      return mMediaSeekable;

I think we can delete MediaDecoderStateMachine::mMediaSeekable and MediaDecoderStateMachine::IsMediaSeekable() as well, and everywhere that calls it can just call mReader->IsMediaSeekable().

The only place that can't call mReader->IsMediaSeekable() is MediaDecoderStateMachine::Seek() because that's called on the main thread, and we don't really want to call into the Reader on the main thread. That should call mDecoder->IsMediaSeekable() instead. We shouldn't really call Seek() on a non-seekable state machine, so please also add an appropriate NS_WARNING() in MediaDecoderStateMachine::Seek() if it's called when mDecoder->IsMediaSeekable() is true.

Please also assert in MediaDecoderStateMachine::DecodeSeek() that mReader->IsMediaSeekable().

::: content/media/fmp4/MP4Reader.cpp
@@ +254,5 @@
> +MP4Reader::IsMediaSeekable()
> +{
> +  // We can seek if we get a duration *and* the reader reports that it's
> +  // seekable.
> +  if (!mDecoder->GetResource()->IsTransportSeekable() || !mDemuxer->CanSeek()) {

return mDecoder->GetResource()->IsTransportSeekable() && mDemuxer->CanSeek();

::: content/media/webm/WebMReader.cpp
@@ +476,5 @@
> +// probably be nullptr.
> +bool
> +WebMReader::IsMediaSeekable()
> +{
> +  return nestegg_has_cues(mContext);

return mContext && nestegg_has_cues(mContext);

Then you don't need the warning.
Attachment #8439198 - Flags: review?(cpearce)
Attachment #8439198 - Flags: review-
Attachment #8439198 - Flags: feedback+
Attached patch bug-1015985-fix (obsolete) — Splinter Review
Temporary patch for try tests.
Attachment #8439198 - Attachment is obsolete: true
Attachment #8439759 - Flags: review?(cpearce)
Comment on attachment 8439759 [details] [diff] [review]
bug-1015985-fix

Review of attachment 8439759 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there. Just a few unaddressed review comments.

::: content/media/MediaDecoderStateMachine.cpp
@@ +2145,5 @@
>      ReentrantMonitorAutoExit exitMon(mDecoder->GetReentrantMonitor());
>      res = mReader->ReadMetadata(&info, getter_Transfers(mMetadataTags));
>    }
> +
> +  if (NS_SUCCEEDED(res)) {

You didn't address my review comment.

Please only call "mDecoder->SetMediaSeekable(mReader->IsMediaSeekable());" *after* the block that calls StartWaitForResources(). Not before like you're doing here and in the previous patch.

::: content/media/webm/WebMReader.cpp
@@ +476,5 @@
> +// probably be nullptr.
> +bool
> +WebMReader::IsMediaSeekable()
> +{
> +  return nestegg_has_cues(mContext) && mContext;

Things are evaluated left-to-right here. So if mContext is null, nestegg_has_cues will dereference it and crash.

So make that:

return mContext && nestegg_has_cues(mContext);

Then if mContext is null the express will evaluate to false and the right hand side expression (&& nestegg_has_cues(mContext)) will not be evaluated.
Attachment #8439759 - Flags: review?(cpearce) → review-
Attached patch bug-1015985_v2Splinter Review
(In reply to Chris Pearce (:cpearce) from comment #29)
> 
> You didn't address my review comment.
> 

Sorry, some of your comments were not addressed correctly. 
WebMReader.cpp and MediaDecoderStateMachine.cpp have been changed according to your review comment.
Attachment #8440997 - Flags: review?(cpearce)
Comment on attachment 8440997 [details] [diff] [review]
bug-1015985_v2

Review of attachment 8440997 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Thanks!
Attachment #8440997 - Flags: review?(cpearce) → review+
(In reply to Chris Pearce (:cpearce) from comment #31)
> Comment on attachment 8440997 [details] [diff] [review]
> bug-1015985_v2
> 
> Review of attachment 8440997 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good! Thanks!

Thank you for reviewing the multiple versions of our patch. We really appreciate it.
Attachment #8439759 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/f14b9781345f
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.