Remove AbstractMediaDecoder::CompositorUpdatedEvent()

RESOLVED FIXED in Firefox 56

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(3 attachments)

No description provided.
Assignee: nobody → jwwang
Blocks: 1378295
Priority: -- → P3
Attachment #8885977 - Flags: review?(jyavenard)
Attachment #8885978 - Flags: review?(jyavenard)
Attachment #8885979 - Flags: review?(jyavenard)
Comment on attachment 8885977 [details]
Bug 1380234. P1 - pass KnowsCompositor to MFR through MediaDecoderReaderInit.

https://reviewboard.mozilla.org/r/156758/#review161978

::: dom/media/MediaDecoder.cpp:1064
(Diff revision 1)
>  }
>  
> +already_AddRefed<KnowsCompositor>
> +MediaDecoder::GetCompositor()
> +{
> +  MediaDecoderOwner* owner;

please initialise owner with GetOwner() and rewrap things so that we don't have assignments inside the test.

::: dom/media/MediaDecoderReader.h:72
(Diff revision 1)
>  {
>    AbstractMediaDecoder* const mDecoder;
>    MediaResource* mResource = nullptr;
>    VideoFrameContainer* mVideoFrameContainer = nullptr;
>    FrameStatistics* mFrameStats = nullptr;
> +  already_AddRefed<layers::KnowsCompositor> mKnowsCompositor;

all objects above are also RefCounted objet, yet all are stored as T*

feels a bit bizarre to use already_AddRefed for this new members, but not the others.

::: dom/media/android/AndroidMediaReader.h:44
(Diff revision 1)
>    MozPromiseRequestHolder<MediaDecoderReader::VideoDataPromise> mSeekRequest;
>    RefPtr<VideoFrameContainer> mVideoFrameContainer;
>  
>  public:
>    AndroidMediaReader(const MediaContainerType& aContainerType,
> -                     const MediaDecoderReaderInit& aInit);
> +                     MediaDecoderReaderInit& aInit);

why remove the const here?

::: dom/media/android/AndroidMediaReader.cpp:29
(Diff revision 1)
>  
>  typedef mozilla::layers::Image Image;
>  typedef mozilla::layers::PlanarYCbCrImage PlanarYCbCrImage;
>  
>  AndroidMediaReader::AndroidMediaReader(const MediaContainerType& aContainerType,
> -                                       const MediaDecoderReaderInit& aInit) :
> +                                       MediaDecoderReaderInit& aInit) :

same here
Attachment #8885977 - Flags: review?(jyavenard) → review+
Comment on attachment 8885978 [details]
Bug 1380234. P2 - send compositor updates to the reader directly without using MediaEventSource.

https://reviewboard.mozilla.org/r/156760/#review161980

all of this seems wrong, mKnowsCompositor is updated, but there's no resulting action from it. The created MediaDataDecoder are still using the old KnowsCompositor.

I wonder if that's the reason the fallback to software decoding doesn't work when the GPU process dies.

::: dom/media/MediaDecoderReader.h:140
(Diff revision 1)
>      return OwnerThread()->IsCurrentThreadIn();
>    }
>  
>    void UpdateDuration(const media::TimeUnit& aDuration);
>  
> +  virtual void UpdateCompositor(already_AddRefed<layers::KnowsCompositor>) {}

space between { }
Attachment #8885978 - Flags: review?(jyavenard) → review+
Comment on attachment 8885979 [details]
Bug 1380234. P3 - remove AbstractMediaDecoder::CompositorUpdatedEvent() and its users.

https://reviewboard.mozilla.org/r/156762/#review161982
Attachment #8885979 - Flags: review?(jyavenard) → review+
Comment on attachment 8885977 [details]
Bug 1380234. P1 - pass KnowsCompositor to MFR through MediaDecoderReaderInit.

https://reviewboard.mozilla.org/r/156758/#review161978

> all objects above are also RefCounted objet, yet all are stored as T*
> 
> feels a bit bizarre to use already_AddRefed for this new members, but not the others.

1. Use T* over RefPtr<> to avoid ref-counting overhead since MediaDecoderReaderInit is a wrapper to forward data to another function. That's why we have the MOZ_STACK_CLASS annotation to constrain its life cycle and prevent dangling pointers.
2. When RefPtr<T> is used, the destructor of MediaDecoderReaderInit (which is generated by the compiler in the scope of MediaDecoderReader.h) needs to see the complete definition of T's destructor. Then we will have to include T's header for forward declaration won't work.
3. Since layerManager->AsShadowForwarder() returns a raw pointer, we want to store it in a RefPtr<> to ensure it lives long enough. Following 2, for forward declaration to work, we need to store an already_AddRefed<> instead of a RefPtr<> in MediaDecoderReaderInit.

> why remove the const here?

The constructor of RefPtr<T> can take either |already_AddRefed<I>&| or |already_AddRefed<I>&&|, but not |const already_AddRefed<I>&|. |const MediaDecoderReaderInit| will make |aInit.mKnowsCompositor| a |const already_AddRefed<I>|.
Comment on attachment 8885978 [details]
Bug 1380234. P2 - send compositor updates to the reader directly without using MediaEventSource.

https://reviewboard.mozilla.org/r/156760/#review162344

::: dom/media/MediaDecoderReader.h:140
(Diff revision 1)
>      return OwnerThread()->IsCurrentThreadIn();
>    }
>  
>    void UpdateDuration(const media::TimeUnit& aDuration);
>  
> +  virtual void UpdateCompositor(already_AddRefed<layers::KnowsCompositor>) {}

I did. But it was removed by clang-format. I would prefer clang-format over our existing coding style for I don't want to double check the output of clang-format.
Comment on attachment 8885978 [details]
Bug 1380234. P2 - send compositor updates to the reader directly without using MediaEventSource.

https://reviewboard.mozilla.org/r/156760/#review161980

http://searchfox.org/mozilla-central/rev/cef8389c687203085dc6b52de2fbd0260d7495bf/dom/media/MediaFormatReader.cpp#680
mKnowsCompositor is used next time when creating a video decoder. I guess you will get a decode error when GPU process dies. So next time a software decoder will be created when trying to recover from the decode error.
Thanks for the review!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0e52be8b1d4d
P1 - pass KnowsCompositor to MFR through MediaDecoderReaderInit. r=jya
https://hg.mozilla.org/integration/autoland/rev/610be3ec3cbe
P2 - send compositor updates to the reader directly without using MediaEventSource. r=jya
https://hg.mozilla.org/integration/autoland/rev/494e4e19f0ee
P3 - remove AbstractMediaDecoder::CompositorUpdatedEvent() and its users. r=jya
You need to log in before you can comment on or make changes to this bug.