[CoreIPC] Encoding/decoding of WebCore::CapabilityValueOrRange transmits padding bytes. Currently ValueUnion objects are encoded as a block of memory instead of as individual values. <rdar://problem/75981660>
Note for potential reviewers (especially Alex): WebCore::CapabilityValueOrRange could be changed to be a WTF::Variant, but to keep this patch simple, I started by fixing the issue of sending padding bytes first. Also, it's been long enough since I've worked on CoreIPC that I'm not sure if this is a step forward to the "new" way of writing IPC decoders, but it keeps RealtimeMediaSourceCapabilities::decode() very simple to read by hiding the complexity in the CapabilityValueOrRange::decoder() method and in IPC/ArgumentCoders.h: template<class Decoder> bool RealtimeMediaSourceCapabilities::decode(Decoder& decoder, RealtimeMediaSourceCapabilities& capabilities) { return decoder.decode(capabilities.m_width) && decoder.decode(capabilities.m_height) && decoder.decode(capabilities.m_aspectRatio) && decoder.decode(capabilities.m_frameRate) && decoder.decode(capabilities.m_facingMode) && decoder.decode(capabilities.m_volume) && decoder.decode(capabilities.m_sampleRate) && decoder.decode(capabilities.m_sampleSize) && decoder.decode(capabilities.m_echoCancellation) && decoder.decode(capabilities.m_deviceId) && decoder.decode(capabilities.m_groupId) && decoder.decode(capabilities.m_supportedConstraints); }
Created attachment 424979 [details] Patch v1
(In reply to David Kilzer (:ddkilzer) from comment #2) > Created attachment 424979 [details] > Patch v1 I may have over-used makeOptional() as well. Should I remove every makeOptional() that's not necessary?
Comment on attachment 424979 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=424979&action=review I personally prefer {{ }} over makeOptional. > Source/WebCore/platform/mediastream/RealtimeMediaSourceCapabilities.h:105 > + ValueUnion m_minOrValue; Instead of all this, why don't we just use Variant<double, int, std::pair<double, double>, std::pair<int, int>>? > Source/WebKit/Platform/IPC/ArgumentCoders.h:181 > + optional = U::decode(decoder); Are there types where this doesn't work, or could we just remove the is_default_constructible version of this?
Comment on attachment 424979 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=424979&action=review >> Source/WebCore/platform/mediastream/RealtimeMediaSourceCapabilities.h:105 >> + ValueUnion m_minOrValue; > > Instead of all this, why don't we just use Variant<double, int, std::pair<double, double>, std::pair<int, int>>? See Comment #1. Switching to use a Variant<> was more complexity than I wanted to tackle in this patch. BTW, any advantage to using std::pair<double, double> instead of Vector<double>[2]? I though std::pair<> was most commonly used for two different types. >> Source/WebKit/Platform/IPC/ArgumentCoders.h:181 >> + optional = U::decode(decoder); > > Are there types where this doesn't work, or could we just remove the is_default_constructible version of this? I wouldn't have guessed this, but the is_default_constructible<> version is not used! Will post a new patch.
(In reply to David Kilzer (:ddkilzer) from comment #5) > Comment on attachment 424979 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=424979&action=review > > >> Source/WebCore/platform/mediastream/RealtimeMediaSourceCapabilities.h:105 > >> + ValueUnion m_minOrValue; > > > > Instead of all this, why don't we just use Variant<double, int, std::pair<double, double>, std::pair<int, int>>? > > See Comment #1. Switching to use a Variant<> was more complexity than I > wanted to tackle in this patch. I think it's exactly as much complexity as should be tackled in this patch. > > BTW, any advantage to using std::pair<double, double> instead of > Vector<double>[2]? I though std::pair<> was most commonly used for two > different types. std::pair keeps enough space for two things. Vector keeps a size and a pointer to a dynamically allocated buffer.
(In reply to Alex Christensen from comment #6) > (In reply to David Kilzer (:ddkilzer) from comment #5) > > Comment on attachment 424979 [details] > > Patch v1 > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=424979&action=review > > > > >> Source/WebCore/platform/mediastream/RealtimeMediaSourceCapabilities.h:105 > > >> + ValueUnion m_minOrValue; > > > > > > Instead of all this, why don't we just use Variant<double, int, std::pair<double, double>, std::pair<int, int>>? > > > > See Comment #1. Switching to use a Variant<> was more complexity than I > > wanted to tackle in this patch. > > I think it's exactly as much complexity as should be tackled in this patch. I wrote the patch to switch to Variant, but it turned out a bit bigger than I would have liked (though not too big to review). > > BTW, any advantage to using std::pair<double, double> instead of > > Vector<double>[2]? I though std::pair<> was most commonly used for two > > different types. > > std::pair keeps enough space for two things. Vector keeps a size and a > pointer to a dynamically allocated buffer. Great point. I also used C++17 strutted bindings in one source file to hide the need to use .first and .second. <https://en.cppreference.com/w/cpp/language/structured_binding>
Created attachment 425546 [details] Patch v2
Comment on attachment 425546 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=425546&action=review > Source/WebCore/platform/mediastream/RealtimeMediaSourceCapabilities.h:44 > + using ULong = int; What does the "U" in "ULong" mean? If it means "unsigned" then why is the type "signed int"?
Created attachment 425557 [details] Patch v3
Comment on attachment 425546 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=425546&action=review >> Source/WebCore/platform/mediastream/RealtimeMediaSourceCapabilities.h:44 >> + using ULong = int; > > What does the "U" in "ULong" mean? If it means "unsigned" then why is the type "signed int"? I don't know. One would imagine it means "unsigned" as in "unsigned long", but one would be mistaken because LongRange.idl defines the value as an int. Guess I'll change it to Long/LongRange then! (Not sure why the original enums used "ULong", though.)
Comment on attachment 425546 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=425546&action=review >>> Source/WebCore/platform/mediastream/RealtimeMediaSourceCapabilities.h:44 >>> + using ULong = int; >> >> What does the "U" in "ULong" mean? If it means "unsigned" then why is the type "signed int"? > > I don't know. One would imagine it means "unsigned" as in "unsigned long", but one would be mistaken because LongRange.idl defines the value as an int. > > Guess I'll change it to Long/LongRange then! (Not sure why the original enums used "ULong", though.) I also wondered if I should change "Double" and "Long" to "DoubleValue" and "LongValue" just so they aren't so generic. Feedback welcome.
Created attachment 425558 [details] Patch v4
(In reply to David Kilzer (:ddkilzer) from comment #10) > Created attachment 425557 [details] > Patch v3 Build fix for GTK port. (In reply to David Kilzer (:ddkilzer) from comment #13) > Created attachment 425558 [details] > Patch v4 Rename ULong/ULongRange to Long/LongRange.
Comment on attachment 425558 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=425558&action=review > Source/WebCore/platform/mediastream/RealtimeMediaSourceCapabilities.h:102 > + const CapabilityValueOrRange& width() const { return *m_width; } There is no guarantee that m_width might not be null here.
(In reply to David Kilzer (:ddkilzer) from comment #11) > Comment on attachment 425546 [details] > Patch v2 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=425546&action=review > > >> Source/WebCore/platform/mediastream/RealtimeMediaSourceCapabilities.h:44 > >> + using ULong = int; > > > > What does the "U" in "ULong" mean? If it means "unsigned" then why is the type "signed int"? > > I don't know. One would imagine it means "unsigned" as in "unsigned long", > but one would be mistaken because LongRange.idl defines the value as an int. > > Guess I'll change it to Long/LongRange then! (Not sure why the original > enums used "ULong", though.) Looks like that is my fault. The value was `unsigned long` when I added ValueUnion in r193389: union ValueUnion { unsigned long asULong; float asFloat; }; but when I changed it to `int` in r205348 I neglected to update the enum.
Comment on attachment 425558 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=425558&action=review > Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:319 > + WTFLogAlways("RealtimeMediaSource::supportsSizeAndFrameRate failed width constraint, capabilities are [%d, %d]", *range.min, *range.max); This also looks like it's unchecked dereferencing something. Maybe an Optional. I don't think it should. It should be 0 or the dereferenced value like it was. > Source/WebCore/platform/mediastream/RealtimeMediaSourceCapabilities.h:156 > + Optional<CapabilityValueOrRange> m_width; Why were these changed to Optional?