Closed Bug 1185792 Opened 9 years ago Closed 9 years ago

Enable new WebMDemuxer / MediaFormatReader by default

Categories

(Core :: Audio/Video, defect)

Product:
Core
Component:
Audio/Video
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla42
Tracking Flags:
Tracking Status
firefox42 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(4 files, 5 obsolete files)

Enable WebMDemuxer/MediaFormatReader by default.
1.04 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
Bug-1185792-Don-t-clear-mNeedReIndex-if-GetCachedRanges.patch
1.23 KB, patch
jya
: review+
Details | Diff | Splinter Review
Bug-1185792-AndroidDecoderModule-requires-API-16.patch
819 bytes, patch
j
: review+
Details | Diff | Splinter Review
Bug-1185792-make-MediaInfo.mFrame-nsIntRect.patch
4.81 KB, patch
jya
: review+
Details | Diff | Splinter Review 
It passes all try tests ; it can be enabled by default.
Assignee: nobody → jyavenard
Jan could you have a look at those intermittents failures?
Flags: needinfo?(j)
sorry:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=258ea6110ebf


Assertion failure: mGotTimecodeScale, at /builds/slave/try-lx-d-000000000000000000000/build/src/dom/media/webm/WebMBufferedParser.cpp:172
TEST-UNEXPECTED-TIMEOUT | dom/media/test/test_seek-11.html | application timed out after 330 seconds with no output
TEST-UNEXPECTED-FAIL | dom/media/test/test_seek-11.html | application terminated with exit code 6
PROCESS-CRASH | dom/media/test/test_seek-11.html | application crashed [@ mozilla::WebMBufferedParser::Append(unsigned char const*, unsigned int, nsTArray<mozilla::WebMTimeDataOffset>&, mozilla::ReentrantMonitor&)]
PROCESS-CRASH | dom/media/test/test_seek-11.html | application crashed [@ linux-gate.so + 0x424]
TEST-UNEXPECTED-FAIL | leakcheck | tab process: missing output line for total leaks!
TEST-UNEXPECTED-FAIL | leakcheck | default process: missing output line for total leaks!
R
(In reply to Jean-Yves Avenard [:jya] from comment #2)
> Jan could you have a look at those intermittents failures?

Looks like WebMBufferedState gets out of sync with available data while
running with e10s repeatedly i.e.:

 ./mach mochitest dom/media/test/test_seek-11.html --e10s

With the following patch I was not able to trigger this issue anymore.
Flags: needinfo?(j)
Attachment #8636782 - Flags: review?(jyavenard)
Comment on attachment 8636453 [details] [diff] [review]
Enable WebMDemuxer/MediaFormatReader by default.

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

What's blocking enabling it for MSE as well?
Attachment #8636453 - Flags: review?(kinetik) → review+
(In reply to Matthew Gregan [:kinetik] from comment #5)
> Comment on attachment 8636453 [details] [diff] [review]
> Enable WebMDemuxer/MediaFormatReader by default.
> 
> Review of attachment 8636453 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What's blocking enabling it for MSE as well?

It triggers some weird crashes on Android.
Going to enable the new MSE and see if it happens there as well. I've been concentrating on getting the new MSE enabled by default first.
Comment on attachment 8636782 [details] [diff] [review]
Fix-e10s-crashes-in-WebMDemuxer.patch

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

r=me with question addressed.

::: dom/media/webm/WebMDemuxer.cpp
@@ +900,4 @@
>  WebMTrackDemuxer::Reset()
>  {
>    mSamples.Reset();
> +  mParent->NotifyDataRemoved();

I don't understand why this would be required.

Reset() is only called when we're about to seek. The aim is to cancel current ongoing operations.

That and the name of the function for its use is now extremely confusing. We're not removing any data from the resource.
Attachment #8636782 - Flags: review?(jyavenard) → review+
Comment on attachment 8636782 [details] [diff] [review]
Fix-e10s-crashes-in-WebMDemuxer.patch

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

That code path doesn't make sense to me.

In InitBufferedState ; with mNeedReIndex being true upon construction ; EnsureUpToDateIndex() will always be run and update the index.

For this to not run:
  if (mInitData && mBufferedState->GetInitEndOffset() == -1) {
    mBufferedState->NotifyDataArrived(mInitData->Elements(), mInitData->Length(), 0);
  }

would mean that   nsresult rv = resource->GetCachedRanges(byteRanges);
returned a failure.

If that's the case, a better fix would be to simply move 
  if (mInitData && mBufferedState->GetInitEndOffset() == -1) {
    mBufferedState->NotifyDataArrived(mInitData->Elements(), mInitData->Length(), 0);
  }
(in EnsureUpToDateIndex) at the beginning.
I can confirm that changing just the InitBufferedState() works now locally.

So to me, this only indicates that it only works because we've added a small delay on when the index will be updated, and that doesn't fix the root cause.

Maybe mBufferedState->GetInitEndOffset() doesn't always returns -1 when it should ; but how could that be?
Looking into this more I think I found the issue:

What happens is that GetCachedRanges does not include the init segment but WebMBufferedState needs the init segment before other parts can be parsed. EnsureUpToDateIndex will now return if this is the case.
Attachment #8636782 - Attachment is obsolete: true
Attachment #8637214 - Flags: review?(jyavenard)
Comment on attachment 8637214 [details] [diff] [review]
Bug-1185792-WebMBufferedState-needs-init-segment.patch

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

So the Demuxer will now stop indexing forever?
The webm parser won't be recreated when that happens, so this condition is now permanent. That can't be right.

With MSE, the init segment is the first thing that usually gets evicted. So having the cached buffered range not containing the init segment is a common occurrence.
(In reply to Jean-Yves Avenard [:jya] from comment #12)
> Comment on attachment 8637214 [details] [diff] [review]
> Bug-1185792-WebMBufferedState-needs-init-segment.patch
> 
> Review of attachment 8637214 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So the Demuxer will now stop indexing forever?

only if it never gets the init segment, which is needed for the index.

> The webm parser won't be recreated when that happens, so this condition is
> now permanent. That can't be right.
> 
> With MSE, the init segment is the first thing that usually gets evicted. So
> having the cached buffered range not containing the init segment is a common
> occurrence.

If Clone is used the init segment gets passed on (MSE),
this if for the case that state never got the init segment.
(In reply to Jan Gerber from comment #13)
> (In reply to Jean-Yves Avenard [:jya] from comment #12)
> > Comment on attachment 8637214 [details] [diff] [review]
> > Bug-1185792-WebMBufferedState-needs-init-segment.patch
> > 
> > Review of attachment 8637214 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > So the Demuxer will now stop indexing forever?
> 
> only if it never gets the init segment, which is needed for the index.

Then I would prefer your first fix (and the one I push to try). It's much easier to understand then.

> If Clone is used the init segment gets passed on (MSE),
> this if for the case that state never got the init segment.

But this would only be the case for the Clone() right? in which case, passing the init segment right at the beginning of the initialization of the WebMBufferedParser is the most elegant way to do so.
(In reply to Jean-Yves Avenard [:jya] from comment #14)
> (In reply to Jan Gerber from comment #13)
> > (In reply to Jean-Yves Avenard [:jya] from comment #12)
> > > So the Demuxer will now stop indexing forever?
> > 
> > only if it never gets the init segment, which is needed for the index.
> 
> Then I would prefer your first fix (and the one I push to try). It's much
> easier to understand then.

That alone broke thew new MSE code path for me. The updated patch works for both.
EnsureUpToDateIndex is called on Init and Clone (via InitBufferedState) and uses
existing mInitData or a buffered init segment (MSE creates a demuxer with just
the init segment as resource).
I'm just not convinced that this patch fix the actual problem and instead just apply some band-aid so the problem is bypassed.

While I admit the patch does fix the assert, i see no reason why it should
Attachment #8637214 - Flags: review?(jyavenard) → review-
Ok, lets try this again:
In case GetCachedRanges returns no byte ranges mNeedReIndex got set to false. Instead it should stay so the next call to GetBuffered updates the index. With that change I was not able to trigger the crash anymore.
Attachment #8637214 - Attachment is obsolete: true
Attachment #8640563 - Flags: review?(jyavenard)
Comment on attachment 8640563 [details] [diff] [review]
Bug-1185792-Don-t-clear-mNeedReIndex-if-GetCachedRanges.patch

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

that sounds more like the real solution :)

still seems to indicate something isn't quite parsed right in the buffer parser when passed an empty range.
Attachment #8640563 - Flags: review?(jyavenard) → review+
(In reply to Jean-Yves Avenard [:jya] from comment #18)
> still seems to indicate something isn't quite parsed right in the buffer
> parser when passed an empty range.

It can handle the empty range, the problem was setting mNeedReIndex to false after "parsing" the empty range.
Sure, but it should receive the appropriate NotifyDataArrived when more data comes, so either it doesn't receive the right values or it doesn't handle it properly.
There's still something quite amiss there.

Will push a try test soon.
I'm guessing this will get backed out.

Suspicious things:
 REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/webm-video/offset-1.xhtml | image comparison (==), max difference: 255, number of differing pixels: 672
1000891 Intermittent offset-1.xhtml | image comparison (==), max difference: 255, number of differing pixels: 620367 

And lots of crashes similar:
PROCESS-CRASH | dom/media/test/test_texttrack_chrome.html | application crashed [@ libdvm.so + 0x4012c]

 PROCESS-CRASH | dom/media/test/test_VideoPlaybackQuality.html | application crashed [@ libdvm.so + 0x4012c]

1182285 TEST-UNEXPECTED-FAIL | dom/media/test/test_VideoPlaybackQuality.html | creationTime should be in the past - expected PASS
Jan, could the android crash be the same issue as that other bug mp3 on the Samsung galaxy?

With the new webm enabled ; it happened now to query the default android PDM if vpX / others are supported.
can't think of another reason it could be that explain a crash
Thinking about it.

It's likely the same thing that cause Android <= 16 to be disabled for mp4:

like MP4Decoder.cpp:203:
  // We need android.media.MediaCodec which exists in API level 16 and higher.
  return AndroidBridge::Bridge() && (AndroidBridge::Bridge()->GetAPIVersion() >= 16);
(In reply to Jean-Yves Avenard [:jya] from comment #26)
> Thinking about it.
> 
> It's likely the same thing that cause Android <= 16 to be disabled for mp4:
> 
> like MP4Decoder.cpp:203:
>   // We need android.media.MediaCodec which exists in API level 16 and
> higher.
>   return AndroidBridge::Bridge() &&
> (AndroidBridge::Bridge()->GetAPIVersion() >= 16);

sounds like that is the issue, checking for api 16 in CreateDecoder
Attachment #8640944 - Flags: review?(jyavenard)
Comment on attachment 8640944 [details] [diff] [review]
Bug1185792-CreateDecoderByType-API-16.patch

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

I don't know enough about this code to disable it altogether.

It was only used for h264/mp3/aac until now.

Look at the approach I've taken in the try commit ; I'll let it run its course and see how it goes.

We still have the ref test to check (pixels not matching).
Those references images can be refreshed, though I have no idea how.
Attachment #8640944 - Flags: review?(jyavenard)
(In reply to Jean-Yves Avenard [:jya] from comment #29)
> Comment on attachment 8640944 [details] [diff] [review]
> Bug1185792-CreateDecoderByType-API-16.patch
> 
> Review of attachment 8640944 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't know enough about this code to disable it altogether.
> 
> It was only used for h264/mp3/aac until now.

audio/mpeg would still need to be added as done in Bug 1188870.

CreateDecoderByType is the Android API call that was only added in 16.
Instead of catching it in various places higher up I thought it might
be best to just not call it < 16. But we can move this to another
bug if your try run succeeds.

> We still have the ref test to check (pixels not matching).
> Those references images can be refreshed, though I have no idea how.

Any way to see the results to see how different they are?
(In reply to Jan Gerber from comment #30)
> (In reply to Jean-Yves Avenard [:jya] from comment #29)
> > Comment on attachment 8640944 [details] [diff] [review]
> > Bug1185792-CreateDecoderByType-API-16.patch
> > 
> > Review of attachment 8640944 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I don't know enough about this code to disable it altogether.
> > 
> > It was only used for h264/mp3/aac until now.
> 
> audio/mpeg would still need to be added as done in Bug 1188870.
> 
> CreateDecoderByType is the Android API call that was only added in 16.
> Instead of catching it in various places higher up I thought it might
> be best to just not call it < 16. But we can move this to another
> bug if your try run succeeds.

Oh, is that so ?

Ah yes, calling a non existing api would certainly be a problem !
your patch would be much better then

> 
> > We still have the ref test to check (pixels not matching).
> > Those references images can be refreshed, though I have no idea how.
> 
> Any way to see the results to see how different they are?

you can run the test. I can't remember what command it is though
some ./mach thing
Comment on attachment 8640944 [details] [diff] [review]
Bug1185792-CreateDecoderByType-API-16.patch

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

LGTM, but passing the review to snorp.
Attachment #8640944 - Flags: review?(snorp)
oh well, that was a poor attempt, android tests didn't get to run:

a different way:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a97c8ec12c2a
Comment on attachment 8640944 [details] [diff] [review]
Bug1185792-CreateDecoderByType-API-16.patch

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

::: dom/media/platforms/android/AndroidDecoderModule.cpp
@@ +37,4 @@
>  static MediaCodec::LocalRef CreateDecoder(const nsACString& aMimeType)
>  {
>    MediaCodec::LocalRef codec;
> +  if (AndroidBridge::Bridge() && (AndroidBridge::Bridge()->GetAPIVersion() >= 16)) {

Can we #define the magic version number. I know I've been guilty of this, but we should stop.
Attachment #8640944 - Flags: review?(snorp) → review+
https://hg.mozilla.org/mozilla-central/rev/bce148786433
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Depends on: 1189602
Comment on attachment 8640944 [details] [diff] [review]
Bug1185792-CreateDecoderByType-API-16.patch

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

I think this should be changed so that SupportsMimeType returns false whenever the API is <= 16 ; even for h264, aac and mp3. Because otherwise we would return true here and it will crash later.

Currently, the test to not call SupportsMimeType is done in the MP4Decoder and in MediaSource.cpp . This test should be removed and have the Android PDM always return false so it always fall back to the agnostic PDM.
(In reply to Jean-Yves Avenard [:jya] from comment #36)
> Comment on attachment 8640944 [details] [diff] [review]
> Bug1185792-CreateDecoderByType-API-16.patch
> 
> Review of attachment 8640944 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this should be changed so that SupportsMimeType returns false
> whenever the API is <= 16 ; even for h264, aac and mp3. Because otherwise we
> would return true here and it will crash later.

ok moving test to SupportsMimeType.

> Currently, the test to not call SupportsMimeType is done in the MP4Decoder
> and in MediaSource.cpp . This test should be removed and have the Android
> PDM always return false so it always fall back to the agnostic PDM.

currently GetAPIVersion is called in MP4Decoder and MP3Decoder.
If that needs to be changed, it should go into another bug.

carrying r+.

reopening bug since only one out of 3 patches is currently applied.
Attachment #8640944 - Attachment is obsolete: true
Flags: needinfo?(jyavenard)
Attachment #8642407 - Flags: review+
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8642407 [details] [diff] [review]
Bug-1185792-AndroidDecoderModule-requires-API-16.patch

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

::: dom/media/platforms/android/AndroidDecoderModule.cpp
@@ +257,5 @@
>  {
> +  if (!AndroidBridge::Bridge() || (AndroidBridge::Bridge()->GetAPIVersion() < 16)) {
> +    return false;
> +  }
> +  if (aMimeType.EqualsLiteral("audio/mpeg") ||

test for audio/mpeg should be in the samsung bug. the change is outside the scope of this bug
(In reply to Jean-Yves Avenard [:jya] from comment #38)
> Comment on attachment 8642407 [details] [diff] [review]
> Bug-1185792-AndroidDecoderModule-requires-API-16.patch
> 
> Review of attachment 8642407 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/platforms/android/AndroidDecoderModule.cpp
> @@ +257,5 @@
> >  {
> > +  if (!AndroidBridge::Bridge() || (AndroidBridge::Bridge()->GetAPIVersion() < 16)) {
> > +    return false;
> > +  }
> > +  if (aMimeType.EqualsLiteral("audio/mpeg") ||
> 
> test for audio/mpeg should be in the samsung bug. the change is outside the
> scope of this bug

removed.
Attachment #8642419 - Flags: review+
Attachment #8642407 - Attachment is obsolete: true
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=832cfdc86807

that will probably still have failure to to the reftest pixels change.

I wonder if this could be related with the issue that the last frame isn't output ...
Flags: needinfo?(jyavenard)
this fixes ./layout/reftests/webm-video/offset-1.xhtml
Attachment #8644286 - Flags: review?(jyavenard)
Comment on attachment 8644286 [details] [diff] [review]
Bug-1185792-crop-decoded-image-to-visible-frame.patch

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

it's a bit ugly :)

There's no way the vpx decoder can retrieve this information from the stream ?
Attachment #8644286 - Flags: review?(jyavenard) → review+
this patch doesn't apply on the current tree ; can't test
Comment on attachment 8644286 [details] [diff] [review]
Bug-1185792-crop-decoded-image-to-visible-frame.patch

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

Actually sorry, I changed my mind.

We currently have everything already to pass that information in the VideoDecoderConfig

There are two members: mDisplay and mImage. mDisplay is a nsIntSize and so is mImage ; make that a nsIntRect instead and use that to pass the information.

this would be usable by other video codecs, including mp4 which currently forces us to rely into decoding the SPS ... I'll fix that later.

The only place VideoDecoderConfig::mImage is set is in the H264Wrapper, set x and y at 0 for the time being.

::: dom/media/platforms/agnostic/VPXDecoder.cpp
@@ +45,5 @@
> +    uint8_t *p = mInfo.mExtraData->Elements();
> +    mPicture = IntRect(BigEndian::readUint32(p + (sizeof(uint32_t) * 0)),
> +                       BigEndian::readUint32(p + (sizeof(uint32_t) * 1)),
> +                       BigEndian::readUint32(p + (sizeof(uint32_t) * 2)),
> +                       BigEndian::readUint32(p + (sizeof(uint32_t) * 3)));

I think this is going to bite us later should any other container supporting VPX decides to add data to extradata.

@@ +145,5 @@
>        return -1;
>      }
>  
> +
> +    IntRect picture = mPicture;

you don't need picture anymore, can continue using mPicture.
Attachment #8644286 - Flags: review+ → review-
(In reply to Jean-Yves Avenard [:jya] from comment #45)
> Comment on attachment 8644286 [details] [diff] [review]
> Bug-1185792-crop-decoded-image-to-visible-frame.patch
> 
> Review of attachment 8644286 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Actually sorry, I changed my mind.
> 
> We currently have everything already to pass that information in the
> VideoDecoderConfig
> 
> There are two members: mDisplay and mImage. mDisplay is a nsIntSize and so
> is mImage ; make that a nsIntRect instead and use that to pass the
> information.

that's indeed much nicer.
Attachment #8644286 - Attachment is obsolete: true
Attachment #8644303 - Flags: review?(jyavenard)
Comment on attachment 8644303 [details] [diff] [review]
Bug-1185792-make-MediaInfo.mFrame-nsIntRect.patch

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

::: dom/media/MediaInfo.h
@@ +211,4 @@
>    // Indicates the frame layout for single track stereo videos.
>    StereoMode mStereoMode;
>  
> +  //Visible area of the decoded video's image.

Space before visible.
Attachment #8644303 - Flags: review?(jyavenard) → review+
Depends on: 1192539
Depends on: 1194884
Depends on: 1274748
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: