Closed Bug 1355048 Opened 7 years ago Closed 7 years ago

Add a WebrtcVideoDecoder MediaDataDecoder class.

Categories

(Core :: WebRTC: Audio/Video, enhancement, P2)

Product:
Core
Component:
WebRTC: Audio/Video
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(8 files, 1 obsolete file)

Bug 1355048: P1. Have WebrtcMediaDataDecoder placeholder.
59 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1355048: P2. Remove OMX h264 decoder.
59 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1355048: P3. Remove ImageHandle.
59 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1355048: P4. Remove unnecessary test, and make better use of monitor.
59 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1355048: P5. Remove RenderVideoFrame virtual method.
59 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1355048: P6. Don't use unnecessary reentrant monitor.
59 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1355048: P7. Use Image's dimensions when available.
59 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1355048: P8. Implement WebrtcMediaDataDecoderCodec.
59 bytes, text/x-review-board-request
jesup
: review+
Details 
Have a MediaDataDecoder WebrtcVideoDecoder would allow for universal hardware acceleration on all platforms with such decoder.
Rank: 25
Priority: -- → P2
See Also: → 1377530
Attachment #8882739 - Attachment is obsolete: true
Attachment #8882739 - Flags: review?(rjesup)
Comment on attachment 8882736 [details]
Bug 1355048: P1. Have WebrtcMediaDataDecoder placeholder.

https://reviewboard.mozilla.org/r/153832/#review162540
Attachment #8882736 - Flags: review?(rjesup) → review+
Comment on attachment 8882737 [details]
Bug 1355048: P2. Remove OMX h264 decoder.

https://reviewboard.mozilla.org/r/153834/#review162542
Attachment #8882737 - Flags: review?(rjesup) → review+
Comment on attachment 8882738 [details]
Bug 1355048: P3. Remove ImageHandle.

https://reviewboard.mozilla.org/r/153836/#review162548
Attachment #8882738 - Flags: review?(rjesup) → review+
Comment on attachment 8882740 [details]
Bug 1355048: P4. Remove unnecessary test, and make better use of monitor.

https://reviewboard.mozilla.org/r/153840/#review162550
Attachment #8882740 - Flags: review?(rjesup) → review+
Comment on attachment 8882741 [details]
Bug 1355048: P5. Remove RenderVideoFrame virtual method.

https://reviewboard.mozilla.org/r/153842/#review162552
Attachment #8882741 - Flags: review?(rjesup) → review+
Comment on attachment 8882742 [details]
Bug 1355048: P6. Don't use unnecessary reentrant monitor.

https://reviewboard.mozilla.org/r/153844/#review162554

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2251
(Diff revision 2)
>  private:
>    int width_;
>    int height_;
>    RefPtr<layers::ImageContainer> image_container_;
>    RefPtr<layers::Image> image_;
> -  mozilla::ReentrantMonitor monitor_; // Monitor for processing WebRTC frames.
> +  Mutex mutex_; // Monitor for processing WebRTC frames.

Mutex for processing
Attachment #8882742 - Flags: review?(rjesup) → review+
Comment on attachment 8882743 [details]
Bug 1355048: P7. Use Image's dimensions when available.

https://reviewboard.mozilla.org/r/153846/#review162558
Attachment #8882743 - Flags: review?(rjesup) → review+
Comment on attachment 8882744 [details]
Bug 1355048: P8. Implement WebrtcMediaDataDecoderCodec.

https://reviewboard.mozilla.org/r/153848/#review162560

This is disabled for now - I'm rather concerned from experience about using HW H264 decoders (and encoders!) on Android.  I'm less concerned with VP8 (or VP9) decoders on android, and even H264 may be better now, but there were major problems as of a couple of years ago (B2G, etc).  If so, when we enable it we may need a whitelist (or blacklist) for Android, and (maybe?) for desktop as well, and/or an adaptive "it failed, fall back to SW".  

I should note that we can ride on Chrome's coattails here, since they're using HW encode/decode for Android at least, and maybe for some desktop (I haven't invetigated recently).

::: media/webrtc/signaling/src/media-conduit/WebrtcMediaDataDecoderCodec.h:60
(Diff revision 2)
>      webrtc::DecodedImageCallback* callback) override;
>  
>    int32_t Release() override;
> +
> +private:
> +  ~WebrtcMediaDataDecoder();

virtual?

::: media/webrtc/signaling/src/media-conduit/WebrtcMediaDataDecoderCodec.h:69
(Diff revision 2)
> +
> +  const RefPtr<TaskQueue> mTaskQueue;
> +  const RefPtr<layers::ImageContainer> mImageContainer;
> +  const RefPtr<PDMFactory> mFactory;
> +  RefPtr<MediaDataDecoder> mDecoder;
> +  webrtc::DecodedImageCallback* mCallback = nullptr;

isn't "= nullptr" typically done in the constructor?    Though with c++11 being required now, perhaps that should/is changing.  So if this is c++11 and works on MSVC, fine.

::: media/webrtc/signaling/src/media-conduit/WebrtcMediaDataDecoderCodec.h:73
(Diff revision 2)
> +  MozPromiseRequestHolder<MediaDataDecoder::DecodePromise> mDecodeRequest;
> +  Monitor mMonitor;
> +  // Members below are accessed via mMonitor
> +  MediaResult mError = NS_OK;
> +  MediaDataDecoder::DecodedData mResults;

I would for style add a blank line before Monitor, and say "Members below are protected with mMonitor" or "require holding mMonitor to access".  Minor nit.

::: media/webrtc/signaling/src/media-conduit/WebrtcMediaDataDecoderCodec.cpp:111
(Diff revision 2)
> +  if (mNeedKeyframe) {
> +    if (aInputImage._frameType != webrtc::FrameType::kVideoFrameKey)
> +      return WEBRTC_VIDEO_CODEC_ERROR;
> +    // We have a key frame - is it complete?
> +    if (aInputImage._completeFrame) {
> +      mNeedKeyframe = false;
> +    } else {
> +      return WEBRTC_VIDEO_CODEC_ERROR;
> +    }
> +  }

In theory there are modes in H264 that using non-iframe refresh ("rolling" IDRs if you will), and just guarantee each macroblock will get set within a second or so of starting to receive the stream.  This minimizes channel aquisition time, and avoids spikes in bandwidth.  

This is not often used for video conferences, though is used sometimes for broadcast uses.  I don't think that the webrtc.org code really supports this currently, so it's not much of an issue, but adding a comment that this will need to change if we want to support such streams probably is good.

::: media/webrtc/signaling/src/media-conduit/WebrtcMediaDataDecoderCodec.cpp:122
(Diff revision 2)
> +  RefPtr<MediaRawData> compressedFrame =
> +    new MediaRawData(aInputImage._buffer, aInputImage._length);
> +  if (!compressedFrame->Data()) {
> +    return WEBRTC_VIDEO_CODEC_MEMORY;
> +  }

Can it return a MediaRawData with no Data()?  (I.e. is the allocation of Data() fallible on purpose?)  If so, this is fine.

::: media/webrtc/signaling/src/media-conduit/WebrtcMediaDataDecoderCodec.cpp:168
(Diff revision 2)
> +      webrtc::VideoFrame videoFrame(image,
> +                                    frame->mTime.ToMicroseconds(),
> +                                    frame->mDuration.ToMicroseconds() * 1000,
> +                                    webrtc::VideoRotation::kVideoRotation_0);

Probably rotation should be passed in and applied here.  Just add an XXX comment about it; in general we're not handling rotation currently, and it basically only affects efficiency in some cases for Android (avoids rotating images before encoding or after decoding, in 1 of 4 orientations).

::: media/webrtc/signaling/src/media-conduit/WebrtcMediaDataDecoderCodec.cpp:205
(Diff revision 2)
> +           [&]() {
> +             MonitorAutoLock lock(mMonitor);
> +             done = true;
> +             mMonitor.Notify();
> +           },
> +           []() { MOZ_CRASH("Can't reach here"); });

I find comments like this on MOZ_CRASH don't add anything, personally - on a crash you look at the source line anyways.  There are some cases where a comment in the CRASH is helpful.  This is somewhat personal preference - it adds a (tiny) bit of code bloat.  Totally up to you; keep it as is or use (); either is fine.
Attachment #8882744 - Flags: review?(rjesup) → review+
Comment on attachment 8882744 [details]
Bug 1355048: P8. Implement WebrtcMediaDataDecoderCodec.

https://reviewboard.mozilla.org/r/153848/#review162560

> isn't "= nullptr" typically done in the constructor?    Though with c++11 being required now, perhaps that should/is changing.  So if this is c++11 and works on MSVC, fine.

this is C++11 and works everywhere now (has been for a while) 
. We have preferred C++11 initialisation for a while now, it makes the code easier to read IMHO with only a single place to look into.

> In theory there are modes in H264 that using non-iframe refresh ("rolling" IDRs if you will), and just guarantee each macroblock will get set within a second or so of starting to receive the stream.  This minimizes channel aquisition time, and avoids spikes in bandwidth.  
> 
> This is not often used for video conferences, though is used sometimes for broadcast uses.  I don't think that the webrtc.org code really supports this currently, so it's not much of an issue, but adding a comment that this will need to change if we want to support such streams probably is good.

we can certainly add support to that. there are other places it should be done too, this code was copied from the webrtc one.

> I find comments like this on MOZ_CRASH don't add anything, personally - on a crash you look at the source line anyways.  There are some cases where a comment in the CRASH is helpful.  This is somewhat personal preference - it adds a (tiny) bit of code bloat.  Totally up to you; keep it as is or use (); either is fine.

the shutdown promise can only ever be resolved, never rejected. this is just to indicate that we can never get there.
it's used thorough the code with ShutDown promises.
Comment on attachment 8882744 [details]
Bug 1355048: P8. Implement WebrtcMediaDataDecoderCodec.

https://reviewboard.mozilla.org/r/153848/#review162560

> I would for style add a blank line before Monitor, and say "Members below are protected with mMonitor" or "require holding mMonitor to access".  Minor nit.

done

> Can it return a MediaRawData with no Data()?  (I.e. is the allocation of Data() fallible on purpose?)  If so, this is fine.

MediaRawData::Data() returning nullptr indicates an OOM in the constructor

> Probably rotation should be passed in and applied here.  Just add an XXX comment about it; in general we're not handling rotation currently, and it basically only affects efficiency in some cases for Android (avoids rotating images before encoding or after decoding, in 1 of 4 orientations).

It now returns the rotation value of the input.

> the shutdown promise can only ever be resolved, never rejected. this is just to indicate that we can never get there.
> it's used thorough the code with ShutDown promises.

I've changed it for MOZ_ASSERT_UNREACHABLE(...)
Blocks: 1381478
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5a280a5cc9a0
P1. Have WebrtcMediaDataDecoder placeholder. r=jesup
https://hg.mozilla.org/integration/autoland/rev/24bcb56d3213
P2. Remove OMX h264 decoder. r=jesup
https://hg.mozilla.org/integration/autoland/rev/212bf3d3538c
P3. Remove ImageHandle. r=jesup
https://hg.mozilla.org/integration/autoland/rev/e7f1a5ab2282
P4. Remove unnecessary test, and make better use of monitor. r=jesup
https://hg.mozilla.org/integration/autoland/rev/655f161c7d17
P5. Remove RenderVideoFrame virtual method. r=jesup
https://hg.mozilla.org/integration/autoland/rev/d0237f00a291
P6. Don't use unnecessary reentrant monitor. r=jesup
https://hg.mozilla.org/integration/autoland/rev/49ea8c43becf
P7. Use Image's dimensions when available. r=jesup
https://hg.mozilla.org/integration/autoland/rev/6fa1a719ca74
P8. Implement WebrtcMediaDataDecoderCodec. r=jesup
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: