1186804 - Refactor the NotifyDataArrival code path at MediaResource.cpp and MediaDecoderReader.cpp

Status: RESOLVED WORKSFORME

(Core :: Audio/Video: Playback, defect, P2)

Description

[Benjamin Chen [:bechen]]

see Bug 1184429.

Because the SilentReadAt will fall back to MediaReadAt, the issue of duplicate |NotifyDataArrival| still exist.

For example: STR: seek after metadataloaded
cache block size is 32k

1. At beginning, http download 8k data contains medatada, and fire metadataloaded. (Dispatch a 0~8k NotifyDataArrival task)
2. JS seek to 100k, since 8k < 32k , the first block will drop.
3. NotifyDataArrival wants to parse 0~8k data, trigger "seek to 0" -> "read 0~8k" -> "seek back to 100k"
4. "seek to 0" dispatch a 0~8k |NotifyDataArrival| task again.

So the same offset appears repeatedly until the playback end....

I know the new WebMReadear won't have the problem because the |WebMBufferedState::UpdateIndex| had already record the offset which had processed.
But the other reader such as Gstreamer and DirectShowReader are suffered if play a mp3, their parser doesn't record the offset, they just call |SilentReadAt|. 

Now we have some solution:
1. At ChannelMediaResource::CopySegmentToCache, we copy the data into nsRefPtr<MediaByteBuffer>, and send to each reader (change the NotifyDataArrived to carry it), so we need one extra memcpy on main thread, nut the parser still rins on other thread.

2. Replace the arg |const char *aFromSegment| to |readonly nsRefPtr<xxx>| so that we might keep main thread to do nothing like the current code.

Comment 1

[Jean-Yves Avenard [:jya]]

(In reply to Benjamin Chen [:bechen] from comment #0)
> see Bug 1184429.
> 
> Because the SilentReadAt will fall back to MediaReadAt, the issue of
> duplicate |NotifyDataArrival| still exist.

While yes, the code path exist, and we could logically address.

I've ran for a few days a modified SilentReadAt that assert if the data isn't fully cached. Not once have I entered that code path.
And that makes sense as SilentReadAt is currently always called following a NotifyDataArrived() which indicates that the data hit the cache.

Comment 2

[Benjamin Chen [:bechen]]

(In reply to Jean-Yves Avenard [:jya] from comment #1)
> (In reply to Benjamin Chen [:bechen] from comment #0)
> > see Bug 1184429.
> > 
> > Because the SilentReadAt will fall back to MediaReadAt, the issue of
> > duplicate |NotifyDataArrival| still exist.
> 
> While yes, the code path exist, and we could logically address.
> 
> I've ran for a few days a modified SilentReadAt that assert if the data
> isn't fully cached. Not once have I entered that code path.
> And that makes sense as SilentReadAt is currently always called following a
> NotifyDataArrived() which indicates that the data hit the cache.

I think because my pc runs slowly (vmware 9) so I always hit the code path. And I guess the tryserver are more slower that cause the testcase timeout.

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

It also depends on the download speed. If you seek before gathering 32k bytes (a full media cache block), the metadata will be discarded when seeking happens.

Comment 4

[Benjamin Chen [:bechen]]

Attachment: bug-1186804.v01.patch

The root change of the patch is :
AbstractMediaDecoder.h
virtual void NotifyDataArrived(uint32_t aLength, int64_t aOffset,
MediaByteBuffer* aBuffer, bool aThrottleUpdates) = 0;

Now, only ChannelMediaResource will call the function with a non-null |MediaByteBuffer*| in |ChannelMediaResource::CopySegmentToCache|.

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Can you describe the purpose of this patch?

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8640941 [details] [diff] [review]
bug-1186804.v01.patch

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

Yes, I'd like some background on why you felt that it was necessary.

But at first glance, I don't like it.

First, we're going to allocate memory *very* often, even though the majority of readers make no use NotifyDataArrived ; that alone is a big regression.

My understanding of NotifyDataArrived() is to tell the reader that there's new data in the cache, at that particular offset. As such, it doesn't need a copy *again* of the data that we know is in the mediaresource cache.

On fast networks, we're now talking about having temporarily *twice* the video in ram: one in the mediaresourcecache, and in the MediaByteBuffer.

Your original aim was to not have NotifyDataArrived unnecessarily be called because SilentReadAt() performed a seek. I don't see how this change aims to do that.

The various MP3Reader are going (except maybe the gstreamer one for now), the WebMReader is going ; both already have replacement making use of the MediaFormatReader new architecture, and none of them making use of SilentReadAt. Once those twos are removed, SilentReadAt can also be removed.

I'd prefer to focus on getting this done first .

Or fix SilentReadAt()

::: dom/media/MediaResource.cpp
@@ +458,5 @@
>    CopySegmentClosure* closure = static_cast<CopySegmentClosure*>(aClosure);
>  
> +  nsRefPtr<MediaByteBuffer> buf = new MediaByteBuffer(aCount);
> +  buf->SetLength(aCount);
> +  memcpy(buf->Elements(), aFromSegment, aCount);

AppendElements

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

If SilentReadAt() is gonna to be removed, I think we will be fine without this bug and comment 0 will not be an issue anymore.

Comment 8

[Benjamin Chen [:bechen]]

(In reply to JW Wang [:jwwang] from comment #5)
> Can you describe the purpose of this patch?

The purpose of the patch is to eliminate the redundant NotifyDataArrived/RecreateChannel due to the seek operation (detail in comment 0). At the worst case, the redundant NotifyDataArrived/RecreateChannel will repeatedly appear until playback end.

In the patch, each reader can fall back to SilentReadAt if the MediaByteBuffer* arg is null.

nsRefPtr<MediaByteBuffer> bytes = aBuffer ? aBuffer : mDecoder->GetResource()->SilentReadAt(aOffset, aLength);


(In reply to Jean-Yves Avenard [:jya] from comment #6)
> Comment on attachment 8640941 [details] [diff] [review]
> bug-1186804.v01.patch
> 
> Review of attachment 8640941 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yes, I'd like some background on why you felt that it was necessary.
> 

The redundant NotifyDataArrived/RecreateChannel make the playback performane very worse. Just like an infinite loop, do the NotifyDataArrived/RecreateChannel again and again.

> 
> First, we're going to allocate memory *very* often, even though the majority
> of readers make no use NotifyDataArrived ; that alone is a big regression.
> 
Yes, agree.
So, is it good we create a query method on MediaDecoderReader that tell us the reader needs the data for parser or not?

> 
> The various MP3Reader are going (except maybe the gstreamer one for now),
> the WebMReader is going ; both already have replacement making use of the
> MediaFormatReader new architecture, and none of them making use of
> SilentReadAt. Once those twos are removed, SilentReadAt can also be removed.
> 
> I'd prefer to focus on getting this done first .
> 
> Or fix SilentReadAt()
> 

When we move some readers into MediaFormatReader, there are still 3 readers we need to fix SilentReadAt().
dom/media/apple/AppleMP3Reader.cpp
dom/media/directshow/DirectShowReader.cpp
dom/media/gstreamer/GStreamerReader.cpp

Now I don't have the idea how to fix SilentReadAt().

Comment 9

[Jean-Yves Avenard [:jya]]

dom/media/apple/AppleMP3Reader.cpp can already be removed, it's redundant with the new MP3 player. 

I'll create a bug to remove it.

(In reply to Benjamin Chen [:bechen] from comment #8)
> Now I don't have the idea how to fix SilentReadAt().

I think it's the only good solution to the problem you are trying to solve...

If you want to wrap the problem so it's not important, I feel that patching up the piece of code using SilentReadAt() to dismiss the NotifyDataArrived() if it has already describe an area is a much better approach. Much simpler for a start, and want mess the current architecture (which I think is very nice :))

Comment 10

[Jean-Yves Avenard [:jya]]

maybe the solution is a silentseek instead?

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

Seek is never exposed to the client code of MediaResource. It is done when necessary in response to Read() or ReadAt().

Comment 12

[Jean-Yves Avenard [:jya]]

After discussing with Anthony; were going to take a different approach.

We're going to remove MediaResource::Read/Tell/Seek so we only have MediaResource::ReadAt

And instead have a MediaResource wrapper class that keeps an index and that will offer Read/Seek/Tell access.
You will use this class to access the MediaResource .

This will remove the need for SilentReadAt.

It shouldn't take long to implement

Comment 13

[Eugen Sawin [:esawin]]

I don't think bug 1168435 should be blocking this, the frame parser is independent from MediaResource and the MP3 demuxer only uses MediaResource::ReadAt.

Comment 14

[Jean-Yves Avenard [:jya]]

I've started working on bug 1190238.

This will replace all use of Read/Seek/Tell in all the MP3 and other MediaDecoderReader making use of those.

I will aim to get this uplifted to 41.

Comment 15

[Jean-Yves Avenard [:jya]]

(In reply to Eugen Sawin [:esawin] from comment #13)
> I don't think bug 1168435 should be blocking this, the frame parser is
> independent from MediaResource and the MP3 demuxer only uses
> MediaResource::ReadAt.

That's exactly why it's blocking this bug... Should me moved all MP3 reader to the MP3Demuxer/MediaFormatReader there would be no issue anymore, precisely because it's using ReadAt.

So moving to it would automatically fix part of this bug

Comment 16

[Jean-Yves Avenard [:jya]]

Benjamin, would you be able to test if you can reproduce the issue with the old webm reader now that bug 1190238 has landed:

You'll have to set media.format-reader.webm to false

as the new webm reader didn't check for content like the WebMReader did