Bug 162571 – [MediaStream] Restructure MediaConstraints classes

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

A preliminary review of bug 162147 asked for a restructuring of the MediaConstraint classes: MediaConstraint should not be reference counted, MediaTrackConstraintSetMap should not use a HashMap, clean up MediaConstraint inheritance.

[Eric Carlson]

Created attachment 289859 [details]
Proposed patch.

[Jer Noble]

Comment on attachment 289859 [details]
Proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=289859&action=review

> Source/WebCore/platform/mediastream/MediaConstraints.h:49
> +    static MediaConstraint& create(const AtomicString&);

This is really weird.  Why not just make the constructor public?

> Source/WebCore/platform/mediastream/MediaConstraints.h:55
> +    MediaConstraint(const MediaConstraint& other)
> +    {
> +        m_name = other.m_name;
> +        m_constraintType = other.m_constraintType;
> +        m_dataType = other.m_dataType;
> +    }

Won't this copy-constructor just be auto-generated if you left this out?

> Source/WebCore/platform/mediastream/MediaConstraints.h:242
> +    NumericConstraint(const NumericConstraint& other)
> +        : MediaConstraint(other)
>      {
> +        if (other.m_min)
> +            m_min = other.m_min;
> +        if (other.m_max)
> +            m_max = other.m_max;
> +        if (other.m_exact)
> +            m_exact = other.m_exact;
> +        if (other.m_ideal)
> +            m_ideal = other.m_ideal;
> +    }

Ditto.

> Source/WebCore/platform/mediastream/MediaConstraints.h:282
> -    static Ref<IntConstraint> create(const String& name, MediaConstraintType type) { return adoptRef(*new IntConstraint(name, type)); }
> +    static IntConstraint& create(const AtomicString& name, MediaConstraintType type) { return *new IntConstraint(name, type); }
> +    IntConstraint(const IntConstraint& other)
> +        : NumericConstraint<int>(other)
> +    {
> +    }

Ditto.

> Source/WebCore/platform/mediastream/MediaConstraints.h:302
> -    static Ref<DoubleConstraint> create(const String& name, MediaConstraintType type) { return adoptRef(*new DoubleConstraint(name, type)); }
> +    static DoubleConstraint& create(const AtomicString& name, MediaConstraintType type) { return *new DoubleConstraint(name, type); }
> +    DoubleConstraint(const DoubleConstraint& other)
> +        : NumericConstraint<double>(other)
> +    {
> +    }

Ditto.

> Source/WebCore/platform/mediastream/MediaConstraints.h:318
> +    static BooleanConstraint& create(const AtomicString& name, MediaConstraintType type) { return *new BooleanConstraint(name, type); }

Ditto.

> Source/WebCore/platform/mediastream/MediaConstraints.h:326
> +    BooleanConstraint(const BooleanConstraint& other)
> +        : MediaConstraint(other)
> +    {
> +        if (other.m_exact)
> +            m_exact = other.m_exact;
> +        if (other.m_ideal)
> +            m_ideal = other.m_ideal;
> +    }

Ditto.

> Source/WebCore/platform/mediastream/MediaConstraints.h:389
> +    static StringConstraint& create(const AtomicString& name, MediaConstraintType type) { return *new StringConstraint(name, type); }

Ditto.

> Source/WebCore/platform/mediastream/MediaConstraints.h:397
> +    StringConstraint(const StringConstraint& other)
> +        : MediaConstraint(other)
> +    {
> +        if (!other.m_exact.isEmpty())
> +            m_exact = other.m_exact;
> +        if (!other.m_ideal.isEmpty())
> +            m_ideal = other.m_ideal;
> +    }

It seems like a lot more work to initialize m_exact (e.g.) as empty, and then do an operator= on it, rather than just call the Vector copy-constructor.

> Source/WebCore/platform/mediastream/MediaConstraints.h:502
> +    class iterator {
> +    public:
> +        iterator()
> +            : m_constraint(nullptr)
> +            , m_index(0)
> +        {
> +        }
> +
> +        iterator(const FlattenedConstraint* constraint, size_t index)
> +            : m_constraint(constraint)
> +            , m_index(index)
> +        {
> +        }
> +
> +        MediaConstraint& operator*() const
> +        {
> +            return m_constraint->m_variants.at(m_index).constraint();
> +        }
>  
> -    const_iterator begin() const { return m_constraints.begin(); }
> -    const_iterator end() const { return m_constraints.end(); }
> +        iterator& operator++()
> +        {
> +            m_index++;
> +            return *this;
> +        }
> +
> +        bool operator==(const iterator& other) const { return m_index == other.m_index; }
> +        bool operator!=(const iterator& other) const { return !(*this == other); }

So, you're going to have to be very, very careful with this iterator.  Because adding or removing items from the m_variants list doesn't explicitly invalidate any iterators, you can git into a situation where you walk over uninitialized memory.  Say, you've got something like this:

for (auto iter = flattened.begin(); iter != flattened.end(); ++iter) {
  if (someCondition)
    flattened.merge(someValue);
}

If you're on the last item in flattened, and the merge operation deletes a row, then the next loop comparison will be iter (index=5) != end (index=4).  And it will keep on walking!

One cheap way to catch this kind of thing in debug builds would be for the FlattenedConstraint and it's iterator to both have a "generation" ivar.  And every time you modify m_variants, you bump up the generation on FlattenedConstraint.  The operator*() method on the constraint would ASSERT that its generation matches the FlattenedConstraint.  The whole generation logic could be debug only.

> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:490
> +        advancedConstraint.forEach([&](const MediaConstraint& constraint) {

Woo! I didn't know we added forEach to Vector!

[Eric Carlson]

Created attachment 289972 [details]
Updated patch.

[Eric Carlson]

Created attachment 289977 [details]
Updated patch.

[Eric Carlson]

Committed r206445: https://trac.webkit.org/r206445