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

VERIFIED FIXED in Firefox 48

Status

()

defect
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: gwagner, Assigned: tzimmermann)

Tracking

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

3 years ago
Our emulator builds are broken.
Reporter

Updated

3 years ago
Blocks: 1208371, 1245091
(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.
(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?
Reporter

Comment 5

3 years ago
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
Reporter

Comment 6

3 years ago
It also fails if you build for the device.
Thanks for the error report :-)

I'm trying to build myself, but there's a bit of syncing to do first.
(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.
Reporter

Comment 9

3 years ago
(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.
Andreas, could you specifically comment on the FIXME comment in this patch? I don't know how to fix this. Thanks!
Attachment #8739370 - Flags: feedback?(pehrsons)
Attachment #8738999 - Attachment is obsolete: true
Attachment #8738999 - Flags: review?(anygregor)
Attachment #8739371 - Flags: feedback?(pehrsons)
This patch set fixes building for me.
Reporter

Comment 13

3 years ago
Your patches fix the build for me as well. Thanks Thomas!
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/
Attachment #8739370 - Flags: feedback?(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.
Attachment #8739371 - Flags: feedback?(pehrsons)
See Also: → 1263518
Changes since v1:

  - fixed init and use of mPrincipalHandle
Assignee: nobody → tzimmermann
Attachment #8739370 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8739882 - Flags: review?(rjesup)
Attachment #8739371 - Flags: review?(rjesup)
Attachment #8739882 - Flags: review?(rjesup) → review+
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+
Attachment #8739371 - Flags: review?(rjesup)
(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?
Flags: needinfo?(pehrsons)
https://apprtc.webrtc.org

If sending the camera feed to a remote peer (desktop would be simple) works, we're good.
Flags: needinfo?(pehrsons)
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.
Attachment #8739371 - Flags: review?(rjesup)
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".)
Attachment #8739371 - Flags: review?(rjesup) → review+

Comment 24

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0199400f20b2
https://hg.mozilla.org/mozilla-central/rev/fe1b68c64b74
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(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.
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.