m93 branch-heads/4577.

[cloudwebrtc]

No description provided.

[cloudwebrtc]

I have passed the iOS/macOS full platform and full architecture compilation, we need to confirm the Android platform, and can be merged after the test is correct

[cloudwebrtc]

I tried to compile on windows, android, macOS, and android, everything seems to be ok.

[hiroshihorie]

Great!! yes we should check if our previously applied android patches are still relevant (required).
(I think recent versions of WebRTC already does some better null checking related to simulcast)
Could you take a look @davidliu ? 🙏

[davidliu]

Looks like #11 is still required for NPE handling. IIRC, the patch I pulled that from was fairly recent (Oct 2021ish) so probably isn't in m93.

[davidzhao]

Looks like #11 is still required for NPE handling. IIRC, the patch I pulled that from was fairly recent (Oct 2021ish) so probably isn't in m93.

I believe they are included in this branch (as this is a diff from chrome M93 to main). @davidliu could you confirm this is working as expected for Android? Then we can merge & release.

[davidliu]

Yeah, looked at the branch's code, the appropriate NPE handling is missing (it reverted back to the old non NPE handling code).

[davidzhao]

Yeah, looked at the branch's code, the appropriate NPE handling is missing (it reverted back to the old non NPE handling code).

ah I understand now. @cloudwebrtc could you re-apply #11? Are other patches kept in this PR?

[cloudwebrtc]

@cloudwebrtc @davidliu The problem seems to be more complicated than expected, Some patches are indeed covered

#11
#7

I haven't checked all the patches.

[cloudwebrtc]

This way of maintaining repo does not seem to be perfect. Once the new upstream is merged, the patches we have done will be overwritten without prompting.

[davidliu]

#11 isn't covered.

https://github.com/webrtc-sdk/webrtc/blob/upgrade-to-m93/media/engine/simulcast_encoder_adapter.cc#L717

It's changed since M92, but it still doesn't cover the case where the primary encoder is null.

[cloudwebrtc]

@davidliu I tried to re-add the difference in #11 to this branch, can you review and confirm its correctness?
8bbc6bd

[davidliu]

Checked out the new commit, LGTM.

[davidzhao]

This way of maintaining repo does not seem to be perfect. Once the new upstream is merged, the patches we have done will be overwritten without prompting.

That's a good point.. the other two options are

1. Merge & rebase:
   • have an upstream branch that is always updated to Google's M releases
   • have our changes in main
   • for each release, merge from upstream and rebase our changes on top
   • create release branch for it, that's mostly frozen (since main will be rebased again)
2. Patch files:
   • similar to Shiguredo's approach of keeping .patch files and updating them

Thinking out loud.. perhaps 1. is better than our current approach.. it'll make it clear what we have changed.

[cloudwebrtc]

@hiroshihorie #7 This PR also seems to be covered. We need to confirm whether it affects iOS

[hiroshihorie]

✅