1262808 - B2GOS: dist/include/DOMMediaStream.h:799:3: error: 'PrincipalID' does not name a type

Status: VERIFIED FIXED

(Core :: Audio/Video: MediaStreamGraph, defect)

Description

[Gregor Wagner [:gwagner]]

Our emulator builds are broken.

Comment 1

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1262808 - Fix B2G bustage. r?gwagner

Review commit: https://reviewboard.mozilla.org/r/45011/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45011/

Comment 2

[Andreas Pehrson [:pehrsons]]

I hope that's enough. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f76cae4708e5

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #1)
> Created attachment 8738999 [details]
> MozReview Request: Bug 1262808 - Fix B2G bustage. r?gwagner

Thanks for the fix! I'd like to suggest to use a better patch description.

Comment 4

[Andreas Pehrson [:pehrsons]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #3)
> Thanks for the fix! I'd like to suggest to use a better patch description.

How about "Change leftover PrincipalID to PrincipalHandle for B2G"?

Hmm, can I not build b2g emulator on try anymore?

Comment 5

[Gregor Wagner [:gwagner]]

Thanks for the quick reply!

I still see:
                                              ^
In file included from /Users/gregor/moz/mi/dom/media/webrtc/MediaEngineGonkVideoSource.cpp:4:0,
                 from Unified_cpp_dom_media_webrtc0.cpp:29:
/Users/gregor/moz/mi/dom/media/webrtc/MediaEngineGonkVideoSource.h:68:12: error: 'nsresult mozilla::MediaEngineGonkVideoSource::Start(mozilla::SourceMediaStream*, mozilla::TrackID)' marked override, but does not override
   nsresult Start(SourceMediaStream* aStream, TrackID aID) override;
            ^
/Users/gregor/moz/mi/dom/media/webrtc/MediaEngineGonkVideoSource.h:73:8: error: 'void mozilla::MediaEngineGonkVideoSource::NotifyPull(mozilla::MediaStreamGraph*, mozilla::SourceMediaStream*, mozilla::TrackID, mozilla::StreamTime)' marked override, but does not override
   void NotifyPull(MediaStreamGraph* aGraph,
        ^
In file included from /Users/gregor/moz/mi/dom/media/webrtc/MediaEngineCameraVideoSource.h:8:0,
                 from /Users/gregor/moz/mi/dom/media/webrtc/MediaEngineCameraVideoSource.cpp:5,
                 from Unified_cpp_dom_media_webrtc0.cpp:11:
/Users/gregor/moz/mi/dom/media/webrtc/MediaEngine.h:124:20: error: 'virtual nsresult mozilla::MediaEngineSource::Start(mozilla::SourceMediaStream*, mozilla::TrackID, const PrincipalHandle&)' was hidden [-Werror=overloaded-virtual]
   virtual nsresult Start(SourceMediaStream*, TrackID, const PrincipalHandle&) = 0;
                    ^
In file included from /Users/gregor/moz/mi/dom/media/webrtc/MediaEngineGonkVideoSource.cpp:4:0,
                 from Unified_cpp_dom_media_webrtc0.cpp:29:
/Users/gregor/moz/mi/dom/media/webrtc/MediaEngineGonkVideoSource.h:68:12: error:   by 'nsresult mozilla::MediaEngineGonkVideoSource::Start(mozilla::SourceMediaStream*, mozilla::TrackID)' [-Werror=overloaded-virtual]
   nsresult Start(SourceMediaStream* aStream, TrackID aID) override;
            ^
In file included from /Users/gregor/moz/mi/dom/media/webrtc/MediaEngineCameraVideoSource.h:8:0,
                 from /Users/gregor/moz/mi/dom/media/webrtc/MediaEngineCameraVideoSource.cpp:5,
                 from Unified_cpp_dom_media_webrtc0.cpp:11:
/Users/gregor/moz/mi/dom/media/webrtc/MediaEngine.h:130:16: error: 'virtual void mozilla::MediaEngineSource::NotifyPull(mozilla::MediaStreamGraph*, mozilla::SourceMediaStream*, mozilla::TrackID, mozilla::StreamTime, const PrincipalHandle&)' was hidden [-Werror=overloaded-virtual]
   virtual void NotifyPull(MediaStreamGraph* aGraph,
                ^
In file included from /Users/gregor/moz/mi/dom/media/webrtc/MediaEngineGonkVideoSource.cpp:4:0,
                 from Unified_cpp_dom_media_webrtc0.cpp:29:
/Users/gregor/moz/mi/dom/media/webrtc/MediaEngineGonkVideoSource.h:73:8: error:   by 'void mozilla::MediaEngineGonkVideoSource::NotifyPull(mozilla::MediaStreamGraph*, mozilla::SourceMediaStream*, mozilla::TrackID, mozilla::StreamTime)' [-Werror=overloaded-virtual]
   void NotifyPull(MediaStreamGraph* aGraph,
        ^
In file included from Unified_cpp_dom_media_webrtc0.cpp:29:0:
/Users/gregor/moz/mi/dom/media/webrtc/MediaEngineGonkVideoSource.cpp:67:1: error: prototype for 'void mozilla::MediaEngineGonkVideoSource::NotifyPull(mozilla::MediaStreamGraph*, mozilla::SourceMediaStream*, mozilla::TrackID, mozilla::StreamTime, const PrincipalHandle&)' does not match any in class 'mozilla::MediaEngineGonkVideoSource'
 MediaEngineGonkVideoSource::NotifyPull(MediaStreamGraph* aGraph,
 ^
In file included from /Users/gregor/moz/mi/dom/media/webrtc/MediaEngineGonkVideoSource.cpp:4:0,
                 from Unified_cpp_dom_media_webrtc0.cpp:29:
/Users/gregor/moz/mi/dom/media/webrtc/MediaEngineGonkVideoSource.h:73:8: error: candidate is: void mozilla::MediaEngineGonkVideoSource::NotifyPull(mozilla::MediaStreamGraph*, mozilla::SourceMediaStream*, mozilla::TrackID, mozilla::StreamTime)
   void NotifyPull(MediaStreamGraph* aGraph,
        ^
cc1plus: all warnings being treated as errors

Comment 6

[Gregor Wagner [:gwagner]]

It also fails if you build for the device.

Comment 7

[Andreas Pehrson [:pehrsons]]

Thanks for the error report :-)

I'm trying to build myself, but there's a bit of syncing to do first.

Comment 8

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Gregor Wagner [:gwagner] from comment #6)
> It also fails if you build for the device.

Does this happen on m-c? aries-l just built for me.

Comment 9

[Gregor Wagner [:gwagner]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #8)
> (In reply to Gregor Wagner [:gwagner] from comment #6)
> > It also fails if you build for the device.
> 
> Does this happen on m-c? aries-l just built for me.

I believe its only on inbound right now.

Comment 10

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1262808: Refactor |PrincipalID| to |PrincipalHandle| in Gonk media code

Andreas, could you specifically comment on the FIXME comment in this patch? I don't know how to fix this. Thanks!

Comment 11

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1262808: Fix use of |PrincipleHandle| in |MediaEngineGonkVideoSource|

Comment 12

[Thomas Zimmermann [:tzimmermann] [:tdz]]

This patch set fixes building for me.

Comment 13

[Gregor Wagner [:gwagner]]

Your patches fix the build for me as well. Thanks Thomas!

Comment 14

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8739370 [details] [diff] [review]
[01] Bug 1262808: Refactor |PrincipalID| to |PrincipalHandle| in Gonk media code

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

Thanks Thomas!

Sorry again for the bustage.

I have tried building for b2g in my linux VM but I haven't cleared all issues before getting to gecko on it yet.

::: dom/media/DOMMediaStream.cpp
@@ +1357,5 @@
>      return;
>    }
> +  // FIXME: |doc->GetPrincipal| doesn't exist; clear mPrincipalHandle
> +  //mPrincipalHandle = MakePrincipalHandle(doc->GetPrincipal());
> +  mPrincipalHandle = PRINCIPAL_HANDLE_NONE;

Make this whole ifdef block this:

>   if (!mWindow) {
>     NS_ERROR("Expected window here.");
>     mPrincipalHandle = PRINCIPAL_HANDLE_NONE;
>     return;
>   }
>   nsIDocument* doc = mWindow->GetExtantDoc();
>   if (!doc) {
>     NS_ERROR("Expected document here.");
>     mPrincipalHandle = PRINCIPAL_HANDLE_NONE;
>     return;
>   }
>   mPrincipalHandle = MakePrincipalHandle(doc->NodePrincipal());

Let me know if it doesn't work.

@@ +1466,4 @@
>    mozilla::gfx::IntSize size = image->GetSize();
>    VideoSegment segment;
>  
> +  segment.AppendFrame(image.forget(), delta, size, PRINCIPAL_HANDLE_NONE);

s/PRINCIPAL_HANDLE_NONE/mPrincipalHandle/

@@ +1502,4 @@
>    mozilla::gfx::IntSize size = image->GetSize();
>    VideoSegment segment;
>  
> +  segment.AppendFrame(image.forget(), delta, size, PRINCIPAL_HANDLE_NONE);

s/PRINCIPAL_HANDLE_NONE/mPrincipalHandle/

Comment 15

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8739371 [details] [diff] [review]
[02] Bug 1262808: Fix use of |PrincipleHandle| in |MediaEngineGonkVideoSource|

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

Let me know how this works. If you can, please do try a WebRTC (video!) call with a real device.

::: dom/media/webrtc/MediaEngineGonkVideoSource.cpp
@@ +203,5 @@
>  }
>  
>  nsresult
> +MediaEngineGonkVideoSource::Start(SourceMediaStream* aStream, TrackID aID,
> +                                  const PrincipalHandle& aPrincipalHandle)

We're accessing mPrincipalHandles in MediaEngineGonkVideoSource. To resolve that we could add this aPrincipalHandle to mPrincipalHandles like we do in MediaEngineRemoteVideoSource.

Instead though, I think we should remove the code accessing mPrincipalHandles, namely (in MediaEngineGonkVideoSource::OnNewMediaBufferFrame):

>     uint32_t len = mSources.Length();
>     for (uint32_t i = 0; i < len; i++) {
>       if (mSources[i]) {
>         // Duration is 1 here.
>         // Ideally, it should be camera timestamp here and the MSG will have
>         // enough sample duration without calling NotifyPull() anymore.
>         // Unfortunately, clock in gonk camera looks like is a different one
>         // comparing to MSG. As result, it causes time inaccurate. (frames be
>         // queued in MSG longer and longer as time going by in device like Frame)
>         AppendToTrack(mSources[i], mImage, mTrackID, 1, mPrincipalHandles[i]);
>       }
>     }

Simply because that's what we did for MediaEngineRemoteVideoSource already - it came down to a perf issue. See https://hg.mozilla.org/mozilla-central/rev/b06d6ff27862.

I'll also file a bug to remove the mPrincipalHandles code from MediaEngineRemoteVideoSource.cpp.

Comment 16

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1262808: Refactor |PrincipalID| to |PrincipalHandle| in Gonk media code (v2)

Changes since v1:

  - fixed init and use of mPrincipalHandle

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8739371 [details] [diff] [review]
[02] Bug 1262808: Fix use of |PrincipleHandle| in |MediaEngineGonkVideoSource|

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

Agree with pehrsons; if that works we're set for r+

Comment 18

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #15)
> Comment on attachment 8739371 [details] [diff] [review]
> [02] Bug 1262808: Fix use of |PrincipleHandle| in
> |MediaEngineGonkVideoSource|
> 
> Review of attachment 8739371 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Let me know how this works. If you can, please do try a WebRTC (video!) call
> with a real device.

I don't understand what you what you what me to do in this review.

If it's only about testing WebRTC, is there a web site for testing this?

Comment 19

[Andreas Pehrson [:pehrsons]]

https://apprtc.webrtc.org

If sending the camera feed to a remote peer (desktop would be simple) works, we're good.

Comment 20

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8739371 [details] [diff] [review]
[02] Bug 1262808: Fix use of |PrincipleHandle| in |MediaEngineGonkVideoSource|

Thanks for the info.

I wish I could test this, but I currently can't setup Wifi (bug 1263859) and the SIM card is not detected (bug 1263861). So the device has no connectivity ATM.

May I ask to land this patch anyway? It's required for building B2G. And it should only affect B2G, so the impact is very small.

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8739371 [details] [diff] [review]
[02] Bug 1262808: Fix use of |PrincipleHandle| in |MediaEngineGonkVideoSource|

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

I'm willing to speculatively fix this for gonk (maybe add a comment to the effect that it's a speculative fix); please test it in a webrtc call as soon as is possible (https://mozilla.github.io/webrtc-landing/pc_test.html; select "One-way".)

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0199400f20b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe1b68c64b74

Comment 23

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thank you! Bug 1263950 is for testing.

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=fe1b68c64b74

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0199400f20b2
https://hg.mozilla.org/mozilla-central/rev/fe1b68c64b74

Comment 25

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Randell Jesup [:jesup] from comment #21)
> Comment on attachment 8739371 [details] [diff] [review]
> [02] Bug 1262808: Fix use of |PrincipleHandle| in
> |MediaEngineGonkVideoSource|
> 
> Review of attachment 8739371 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm willing to speculatively fix this for gonk (maybe add a comment to the
> effect that it's a speculative fix); please test it in a webrtc call as soon
> as is possible (https://mozilla.github.io/webrtc-landing/pc_test.html;
> select "One-way".)

FYI, I verified this patch with the given URL in bug 1263950.

Comment 26

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I also verified that the patch does allow for B2G to show webrtc.  The problem on the device is that it runs out of memory.  Not sure if it's related to this patch or some other reason.