Closed Bug 1394995 Opened 7 years ago Closed 7 years ago

Implement DecoderDoctorLogger

Categories

(Core :: Audio/Video: Playback, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: mozbugz, Assigned: mozbugz)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

This bug will provide the initial implementation of DecoderDoctorLogger, with the goals set in bug 1394993, mainly:
- Logging may be enabled at runtime, by simply installing and running an extension.
- Once enabled, everything is logged, so no information should be missing.
- Logging should have a minimal impact on playback; and of course when logging is disabled there should be almost zero overhead.
- Log messages will be tied to their C++ objects, and objects linked between themselves, such that all messages may be linked to each HTMLMediaElement.

(Later bugs will use it to add logging to the media stack, and to let extensions retrieve these logs.)
JW:

- First, no hurry for this first review, I'm on PTO next week. But I can always answer emails if you have questions.

- To see how DecoderDoctorLogger will be used, here's the current patch touching the media stack classes: [1] (totally separate from MOZ_LOG for now.)

- If you want to experiment with the extension, apply the attached under-review patches, then the try-patch [1], and use the attached extension prototype (unzip it and then follow the instructions at [2]).

Thanks!

[1] https://hg.mozilla.org/try/rev/2e6d256af71e371024e4d3389e9091c0a1588fcc
[2] https://github.com/karolien/media-devtools-panel-react
Comment on attachment 8902518 [details]
Bug 1394995 - Moved DecoderDoctor files to dom/media/doctor/ -

https://reviewboard.mozilla.org/r/174108/#review179398
Attachment #8902518 - Flags: review?(jwwang) → review+
Want to have some discussions about the design before diving into the detail implementation:

1. A lot of code is dedicated to managing the parent-child relationship of log messages. Instead of having a centralized log object to handler requests from all media objects, I would like to have a media element create a log object and share it with its downstream objects. Doing so, it is easy to group messages belonging to the same media element by having downstream objects use the same log object.

2. We need more eyes to review the lock-free queue (DDLogMessagesBufferQueue) since atomics is hard to get right. A generic lock-free queue might be also worth a place in MBFT or XPCOM.
1. I thought about that, especially as I had done something like it for DecoderDoctorDiagnostics.

But I decided against it mainly because some objects can live separately for a "long" time before being linked to others. E.g.: MediaKeys. But I still want to log things before objects get linked, and have these logs go to the proper element.

And maybe in the future we could have other "root" objects, and having this link graph exist outside of the actual class hierarchy makes it possible to adapt to other situations.

Also it felt more intrusive than just log-linking objects at the same time they actually get linked: It would take space in every object, even when logging is disabled.

I admit I'm probably extra biased about it now, because it would be a lot of work to change what I've done.
But I'm always happy to discuss further, you have much more experience of the whole stack, so maybe it would be easier than I fear?


2. Yes, more eyes are always better. I'll get someone from MFBT/XPCOM for that, after you've done a first pass.

I would be happy to create a generic lock-free queue, and in fact I did attempt to create one from my code, but I found that my use-case had some very specific quirks: My queue can never be empty; I only need atomic access when adding a new head, not when removing a tail; Only one thread can actually create a new head, so there's no need to do a compareExchange there.
To add to my first reply:

My philosophy with this logger, is that it should quickly capture a lot of information about the media stack at runtime, and then get out of the way; A background process can later sort through all this information and create a living picture of what happened, without having to know about the hierarchy in advance.

I think it's the most flexible, least intrusive option; and it could theoretically be made more generic, and be adapted to other components in the future.
(In reply to Gerald Squelart [:gerald] from comment #9)
> 1. I thought about that, especially as I had done something like it for
> DecoderDoctorDiagnostics.
> 
> But I decided against it mainly because some objects can live separately for
> a "long" time before being linked to others. E.g.: MediaKeys. But I still
> want to log things before objects get linked, and have these logs go to the
> proper element.

It is easy for MediaKeys and HTMLMediaElement to create their own log objects to begin with, and later to merge them when you want to extract logs from a particular HTMLMediaElement. Merge is easy because each log object stores a sorted list of messages.

> And maybe in the future we could have other "root" objects, and having this
> link graph exist outside of the actual class hierarchy makes it possible to
> adapt to other situations.

Distributed log objects don't need "root" at all. Messages are grouped by sharing a single log object. Advanced grouping can be done by merging multiple log objects. This is particularly useful for MediaKeys and HTMLMediaElement which are created and live independently for a long time and then bind together somewhere along the way.
(In reply to Gerald Squelart [:gerald] from comment #10)
> My philosophy with this logger, is that it should quickly capture a lot of
> information about the media stack at runtime, and then get out of the way; A
> background process can later sort through all this information and create a
> living picture of what happened, without having to know about the hierarchy
> in advance.

I think I share the same philosophy. It is just that we differ in how messages are linked and grouped to present all the info about a media element.
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #11)

What about my concern (in comment 9), that having shared log objects would take space in every media stack object, even when logging is disabled?

> (In reply to Gerald Squelart [:gerald] from comment #9)
> ...
> > And maybe in the future we could have other "root" objects, and having this
> > link graph exist outside of the actual class hierarchy makes it possible to
> > adapt to other situations.
> 
> Distributed log objects don't need "root" at all. Messages are grouped by
> sharing a single log object. Advanced grouping can be done by merging
> multiple log objects. This is particularly useful for MediaKeys and
> HTMLMediaElement which are created and live independently for a long time
> and then bind together somewhere along the way.

My concern here is that if we use shared log objects *instead* of parsing link messages from the logs, we will lose this valuable link-graph information -- which I argue could later be used to isolate different groups of objects than just what is tied to a media element.

And if you were thinking of doing both, we would be adding memory usage, even when logging is disabled, that duplicates the information from the linking log messages; with the only advantage being that it's somewhat easier to gather logs for a given media element.


So I'm still keen on my solution.
Maybe I'm missing something? Here's how I think we would implement your proposal, please let me know if I've got something wrong:
A. All media stack objects would inherit from something like: `struct Logged { LogId mLogId; void SetLogId(...); void Log(...); }`.
B. When a "root" object (e.g., HTMLMediaElement, MediaKeys) is created, it creates its own LogId.
C. When an object A creates (or receives?) a sub-object B, A does a SetLogId on B. (Does this log a "link message" too?)
D. Objects just call Log() to log things.
E. If LogId is set, messages directly go to their final storage.
F. If the LogId is not set yet, messages are kept separately. Later, when SetLogId() is called, messages are moved to the appropriate master log.
G. We'll need some way to tie LogIds together (e.g., HTMLMediaElement<->MediaKeys) and gather all their messages together for output.

(Let's keep discussing on Friday)
Attachment #8902519 - Flags: review?(jwwang)
Attachment #8902946 - Flags: review?(jwwang)
After discussing with Chris, there are other big-picture questions that need resolving, to ensure that whatever we implement actually helps with debugging, both internally and in the field; e.g.: Combine logs from multiple processes, keep full logs from crashed processes, interaction with existing MOZ_LOG's, etc.

So I'm retracting the r? for now.
JW, if you had other comments in mind about what you've seen already, please feel free to dump them here so they're not lost.

I will be working on a document where we can discuss goals and architectural details.
Attachment #8902946 - Attachment is obsolete: true
Continuing the review, no big change (the "big-picture questions" don't affect this first part).

JW, I'm sticking with having object-linking information as part of the log (and not in shared C++ objects), I think I've explained my reasons in above comments, and I still believe it's the cheaper and more flexible solution; but at least I hope you can agree that it's a reasonable architecture.
Comment on attachment 8902519 [details]
Bug 1394995 - Initial implementation of DecoderDoctorLogger -

https://reviewboard.mozilla.org/r/174110/#review184174

::: dom/media/doctor/DDLogMessagesBufferQueue.h:22
(Diff revision 3)
> +// a unique DDMessageIndex, and then forward that call to the appropriate
> +// buffer.
> +// New buffers are created when the first message for that buffer arrives;
> +// almost-simultaneous logs will need to wait for the buffer to be created, but
> +// most calls just go through to the buffer with no wait.
> +class DDLogMessagesBufferQueue

It might be a good idea to extract this class to another patch so it is easier to draw eyes of other XPCOM/MFBT peers.

Some other thoughts:
1. Make DDLogMessagesBufferQueue reusable outside the media context. It is also easier to reason about the thread safety/correctness without looking at its client code.
2. Add comparison operators to DDMessageIndex for it is more readable to say:
   DDMessageIndex i1;
   DDMessageIndex i2;
   if (i1 >= i2)
3. Replace GetTailBuffer()/DeleteTailBuffer() with typical Peek/Pop methods to a queue and make them thread safe. It will be much easier to reason about the thread safety/correctness without looking at its client code.
More to come after I digested other DD classes. :)
Comment on attachment 8902519 [details]
Bug 1394995 - Initial implementation of DecoderDoctorLogger -

https://reviewboard.mozilla.org/r/174110/#review184174

> It might be a good idea to extract this class to another patch so it is easier to draw eyes of other XPCOM/MFBT peers.
> 
> Some other thoughts:
> 1. Make DDLogMessagesBufferQueue reusable outside the media context. It is also easier to reason about the thread safety/correctness without looking at its client code.
> 2. Add comparison operators to DDMessageIndex for it is more readable to say:
>    DDMessageIndex i1;
>    DDMessageIndex i2;
>    if (i1 >= i2)
> 3. Replace GetTailBuffer()/DeleteTailBuffer() with typical Peek/Pop methods to a queue and make them thread safe. It will be much easier to reason about the thread safety/correctness without looking at its client code.

Thanks for the review so far, much appreciated.

Ok I'll move DDLogMessagesBufferQueue to a separate patch.

To your thoughts:
1. I hesitate to make it more generic, because it's tailored to my particular use (multi-threaded sub-node front-writes, single-threaded whole-node back-reads, invariant that the head node cannot be pop'ed, maintenance of atomic sub-node counter); But I'll think about it... (Maybe it would be possible to combine DDLogMessagesBufferQueue with DDLogMessagesBuffer to expose that generic queue.)
2. Good idea, thanks.
3. Renaming to Peek/Pop would be fine. But adding thread safety would be useless in my case (single-threaded back-reader); also Peek() could not be 100% safe anyway, e.g.: the caller could keep the Peek()'d pointer, call Pop(), and then continue accessing the now-dangling pointer. But I'll think about that, along with #1.
Comment on attachment 8902519 [details]
Bug 1394995 - Initial implementation of DecoderDoctorLogger -

https://reviewboard.mozilla.org/r/174110/#review184174

> Thanks for the review so far, much appreciated.
> 
> Ok I'll move DDLogMessagesBufferQueue to a separate patch.
> 
> To your thoughts:
> 1. I hesitate to make it more generic, because it's tailored to my particular use (multi-threaded sub-node front-writes, single-threaded whole-node back-reads, invariant that the head node cannot be pop'ed, maintenance of atomic sub-node counter); But I'll think about it... (Maybe it would be possible to combine DDLogMessagesBufferQueue with DDLogMessagesBuffer to expose that generic queue.)
> 2. Good idea, thanks.
> 3. Renaming to Peek/Pop would be fine. But adding thread safety would be useless in my case (single-threaded back-reader); also Peek() could not be 100% safe anyway, e.g.: the caller could keep the Peek()'d pointer, call Pop(), and then continue accessing the now-dangling pointer. But I'll think about that, along with #1.

JW, I managed to extract the queue class, thank you for pushing me. :-)
As part of that, I've also extracted DDMessageIndex into a more generic number class.
Nathan:
I'm proposing a couple of new classes for MFBT:
- A special number class with an ordering-across-overflow feature that works well for things like queue indices,
- A queue specialized for fast thread-safe writing.

Please reassign to someone else as appropriate/convenient.
Let me know if MFBT is not the best place for these.
Thank you.
Comment on attachment 8909186 [details]
Bug 1394995 - RollingNumber -

https://reviewboard.mozilla.org/r/180768/#review185854

::: mfbt/RollingNumber.h:26
(Diff revision 1)
> +// 0 < 1, 2^23 < 2^24
> +// However if the distance is more than half the range, we assume that we are going around the ring,
> +// and therefore consider the smaller number to actually be greater!
> +// uint(-1) < 0.
> +template<typename T>
> +class RollingNumber

I would like to discuss the usage of RN.

E.g. for |r1 < r2|, will |r1/10 < r2/10| still hold?

To avoid such ambiguity, we can:
1. Provide a generator to control how a rolling number is generated.
2. Provide only comparison operators which are all we need to check the order of 2 RNs.
3. Do we need to convert a RN to its underlying integer type? When is this conversion useful?

::: mfbt/tests/moz.build:46
(Diff revision 1)
>      'TestPair',
>      'TestRange',
>      'TestRefPtr',
>      'TestResult',
>      'TestRollingMean',
> +    'TestRollingNumber',

gtest is the trend, right?
Comment on attachment 8909187 [details]
Bug 1394995 - MultiWriterQueue -

https://reviewboard.mozilla.org/r/180770/#review185874

::: mfbt/MultiWriterQueue.h:56
(Diff revision 1)
> +  // Atomically thread-safe; in the worst case some pushes may be blocked
> +  // while a new buffer is created/reused for them.
> +  // Returns true if a new buffer was created/reused internally; useful if
> +  // caller wants to trigger processing regularly at the most efficient time.
> +  template<typename F>
> +  bool PushF(const F&& aF)

|const F& aF|.

::: mfbt/MultiWriterQueue.h:93
(Diff revision 1)
> +
> +  // Pop all elements before the first invalid one, running aF on each of them
> +  // in FIFO order.
> +  // NOT thread-safe with other PopAll calls, but concurrent pushes are allowed.
> +  template<typename F>
> +  void PopAll(F&& aF)

Might be more efficient to have another head for the reader as we have mMostRecentBuffer for the writers.

::: mfbt/MultiWriterQueue.h:150
(Diff revision 1)
> +
> +private:
> +  // Structure containing the element to be stored, and a validity-marker.
> +  struct BufferedElement
> +  {
> +    T mT;

Might be worth supporting types that can't be default initialized as Maybe<T> does.

::: mfbt/MultiWriterQueue.h:165
(Diff revision 1)
> +    Buffer(Buffer* aOlder, Index aStart)
> +      : mOlder(aOlder)
> +      , mStart(aStart)
> +    {
> +    }
> +    Buffer* mOlder = nullptr;

Make mOlder and mStart const by removing in-class initialization.

::: mfbt/MultiWriterQueue.h:196
(Diff revision 1)
> +
> +  // Discard a fully-read buffer, this implementation just makes it reusable.
> +  // Known downside: We will never destroy buffers while this queue is alive;
> +  // so users should ensure queues are regularly popped, to avoid hanging on
> +  // too much memory between peaks.
> +  void DiscardBuffer(Buffer* aBuffer)

How about calling it 'RecycleBuffer'?

::: mfbt/MultiWriterQueue.h:234
(Diff revision 1)
> +      // We know for sure this `buffer` is the correct one for this index,
> +      // and indicate that we have started a new buffer.
> +      return { buffer->mElements[BufferSize - 1], true };
> +    }
> +
> +    if (MOZ_UNLIKELY(i > lastIndex)) {

I am worried this might result in stack overflow if the thread observing |i == lastIndex| is stalled somehow.
Attachment #8909187 - Flags: review?(jwwang) → review+
Comment on attachment 8909186 [details]
Bug 1394995 - RollingNumber -

https://reviewboard.mozilla.org/r/180768/#review186048

I'm skeptical of the placing of this in MFBT as a general purpose class.  I'm also unsure about the behavior of restricting comparisons to only consider half of the range for overflow detection purposes.  Why not 2/3rds or 3/4ths, or `$FAVORITE_FRACTION` instead?  Why not just a proper overflow counter (which has its own complications if it overflows, of course)?  Or a `RingIndex` type that increments as normal, but can be used to index into a queue with modular arithmetic?  It seems like clients might want to customize this behavior somewhat.

If JW wants to take this for the media code, that's fine, but I'm less enthused about seeing it enshrined in MFBT.

::: mfbt/RollingNumber.h:137
(Diff revision 1)
> +  {
> +    return ValueType(aOther.Value() - Value()) > MidWay;
> +  }
> +
> +private:
> +  static_assert(!std::numeric_limits<T>::is_signed, "");

I think this would be more helpful to have at the beginning of the class, along with an actual error message in the `static_assert`.

::: mfbt/tests/TestRollingNumber.cpp:56
(Diff revision 1)
> +
> +    // Assignment construction.
> +    RN8 n42Assigned = n42;
> +    MOZ_RELEASE_ASSERT(n42Assigned == 42);
> +
> +    // Assignement.

Nit: "Assignment"

::: mfbt/tests/moz.build:46
(Diff revision 1)
>      'TestPair',
>      'TestRange',
>      'TestRefPtr',
>      'TestResult',
>      'TestRollingMean',
> +    'TestRollingNumber',

This would need to be added to `testing/cppunittest.ini` too.  MFBT also has gtests nowadays, so you could add it there instead.
Attachment #8909186 - Flags: review?(nfroyd)
Comment on attachment 8909187 [details]
Bug 1394995 - MultiWriterQueue -

https://reviewboard.mozilla.org/r/180770/#review186052

Again, I'm skeptical of the general-purposeness of this code for MFBT.  For XPCOM, maybe, but MFBT usually includes things that feature relevance to the JS engine, and I'm not seeing places where the JS engine might use this--or I am not sufficiently imaginative enough.

I'm also very skeptical about multi-threaded data structures that don't feature any locks.  I know such things exist, but my last impression of such data structures is that they were basically open research projects.  Things might have changed over the last several years (I am working on old information here!), but they are still new enough I would expect references to "here's how you have some assurance that the algorithm used here is correct" sources.  Do you have such references, or is the algorithm used here home-grown?

I did not try to follow through the code with a fine-toothed comb to check that we didn't have any threading issues, but given some of the issues below, I feel comfortable canceling review even setting aside the issue of whether it belongs in MFBT.

::: mfbt/MultiWriterQueue.h:61
(Diff revision 1)
> +    // Let aF write into it.
> +    aF(elementAndNewBuffer.mElement.mT, i);
> +    // Mark an element valid *after* it has been written, so that the reader
> +    // will see consistent information.
> +    elementAndNewBuffer.mElement.mValid = true;

Perhaps this restriction should be enforced in the BufferedElementAndNewBuffer class somehow?  I don't think you can enforce it completely, but a:

```
  template<typename F> Initialize(F&&);
  template<typename F> Invalidate(F&&);
```

API, maybe with a `bool IsValid()` method, would go some way to centralizing the need to write `mValid` *after* accesses.

::: mfbt/MultiWriterQueue.h:91
(Diff revision 1)
> +  // NOT thread-safe with other PopAll calls, but concurrent pushes are allowed.
> +  template<typename F>
> +  void PopAll(F&& aF)

It's not thread-safe with other `PopAll` calls, but pushes from other threads while we're popping things off are allowed?  That's...kind of unusual, isn't it?  How sure are we that such a model is safe?

I would feel better about this if there were some sign, like `const MutexAutoLock&` or something, that strongly encouraged the programmer to realize that he should provide his own guarantees around non-concurrent access here.  Mutex references are not ideal, because that suggests that you can't push to the queue during on-going deletions, but something along those lines.

::: mfbt/MultiWriterQueue.h:192
(Diff revision 1)
> +  // Discard a fully-read buffer, this implementation just makes it reusable.
> +  // Known downside: We will never destroy buffers while this queue is alive;
> +  // so users should ensure queues are regularly popped, to avoid hanging on
> +  // too much memory between peaks.

It seems peculiar to not advise users of this (significant!) downside in the public documentation, or provide some way of addressing it through the public interface.  This is a design just asking to cause memory usage issues.

The last sentence is also inaccurate: "users should ensure queues are regularly popped, to avoid hanging on too (sic) much memory between peaks."  Even if we're popping regularly, we're not actually releasing memory, we're just moving it to the freelist.  So our memory usage is always proportional to the high-water usage of the queue, regardless of the particular sequence of actions taken on the queue.  This is Not Good.

::: mfbt/MultiWriterQueue.h:258
(Diff revision 1)
> +  // Blindly destroy a linked-list of buffers.
> +  void DestroyList(Buffer* aBuffer)

I realize that some of these members are private members, but it would still be good to document all the thread-safety constraints on the private interface as well as the public one.

::: mfbt/tests/TestMultiWriterQueue.cpp:38
(Diff revision 1)
> +  template<size_t BufferSize>
> +  void testQueue(const int loops)

Introducing an ostensibly thread-safe class whose tests don't feature threads in any way seems like very brave programming.  Putting this in a framework that accommodates threads, like gtests, would probably be better, and then rewriting things so we had some kind of multi-threaded access going on would be even better.
Attachment #8909187 - Flags: review?(nfroyd)
Comment on attachment 8909187 [details]
Bug 1394995 - MultiWriterQueue -

https://reviewboard.mozilla.org/r/180770/#review186320

::: mfbt/MultiWriterQueue.h:158
(Diff revision 1)
> +    Atomic<bool> mValid{ false };
> +  };
> +
> +  // Buffer contains a sequence of BufferedElements starting at a specific
> +  // index, and it points to the next-older buffer (if any).
> +  struct Buffer

We can put an atomic counter in struct Buffer, so we don't need mStart and won't worry about overflow (provided BufferSize is much smaller than UINT32_MAX). Then we don't need RollingNumber for MultiWriterQueue.
Comment on attachment 8909187 [details]
Bug 1394995 - MultiWriterQueue -

https://reviewboard.mozilla.org/r/180770/#review186320

> We can put an atomic counter in struct Buffer, so we don't need mStart and won't worry about overflow (provided BufferSize is much smaller than UINT32_MAX). Then we don't need RollingNumber for MultiWriterQueue.

(I'm working on solving the other comments, I'll respond to them later.)

I don't understand what you're suggesting here.
The RollingNumber is used to know which buffer, and which position in the buffer, to write to; and also to ensure that only one push (the one that will write the very last element in the last buffer) can add another buffer.
Could you please elaborate on what you're proposing (pseudo-code?), if you still think it will help? Thanks.
Comment on attachment 8909187 [details]
Bug 1394995 - MultiWriterQueue -

https://reviewboard.mozilla.org/r/180770/#review186320

> (I'm working on solving the other comments, I'll respond to them later.)
> 
> I don't understand what you're suggesting here.
> The RollingNumber is used to know which buffer, and which position in the buffer, to write to; and also to ensure that only one push (the one that will write the very last element in the last buffer) can add another buffer.
> Could you please elaborate on what you're proposing (pseudo-code?), if you still think it will help? Thanks.

struct Buffer
{
  // ...
  Atomic<uint32_t> mNextElementToWrite{ 0 };
}

Buffer* buffer = mMostRecentBuffer;
const uint32_t i = buffer->mNextElementToWrite++;
if (i >= BufferSize) {
  // This buffer is full. Retry until mMostRecentBuffer is updated.
} else if (i == BufferSize - 1) {
  // The last element is claimed.
} else {
  // Now we've claimed an element to write.
}
Comment on attachment 8909187 [details]
Bug 1394995 - MultiWriterQueue -

https://reviewboard.mozilla.org/r/180770/#review186320

> struct Buffer
> {
>   // ...
>   Atomic<uint32_t> mNextElementToWrite{ 0 };
> }
> 
> Buffer* buffer = mMostRecentBuffer;
> const uint32_t i = buffer->mNextElementToWrite++;
> if (i >= BufferSize) {
>   // This buffer is full. Retry until mMostRecentBuffer is updated.
> } else if (i == BufferSize - 1) {
>   // The last element is claimed.
> } else {
>   // Now we've claimed an element to write.
> }

Good idea! I started implementing it, and life was great, making a lot of code clearer...

But I've just realized that it introduces a potential race:
- Current mMostRecentBuffer is B1.
- T1 starts pushing, gets to `i == BufferSize - 1` (i.e., claims the last argument in B1).
- T2 starts pushing, arrives at `Buffer* buffer = mMostRecentBuffer;`, keeping a pointer to B1.
- T1 writes the last element, creates new mMostRecentBuffer B2, marks last element valid.
- T3 does a PopAll, reads the last element in B1, destroys B1 (not yet in this implementation, but it will eventually be done).
- T2 runs its next line `buffer->mNextElementToWrite++` on B1, but B1 has just been destroyed -> UAF.

So I don't think we can safely keep a counter inside buffers, when buffers can be destroyed by another thread.
Do you see a way around that problem?

The current implementation has a single counter per queue, which indicates which buffer can be used by writers, and by design that buffer cannot be destroyed until all its elements have been written and then read.
Comment on attachment 8909187 [details]
Bug 1394995 - MultiWriterQueue -

https://reviewboard.mozilla.org/r/180770/#review186320

> Good idea! I started implementing it, and life was great, making a lot of code clearer...
> 
> But I've just realized that it introduces a potential race:
> - Current mMostRecentBuffer is B1.
> - T1 starts pushing, gets to `i == BufferSize - 1` (i.e., claims the last argument in B1).
> - T2 starts pushing, arrives at `Buffer* buffer = mMostRecentBuffer;`, keeping a pointer to B1.
> - T1 writes the last element, creates new mMostRecentBuffer B2, marks last element valid.
> - T3 does a PopAll, reads the last element in B1, destroys B1 (not yet in this implementation, but it will eventually be done).
> - T2 runs its next line `buffer->mNextElementToWrite++` on B1, but B1 has just been destroyed -> UAF.
> 
> So I don't think we can safely keep a counter inside buffers, when buffers can be destroyed by another thread.
> Do you see a way around that problem?
> 
> The current implementation has a single counter per queue, which indicates which buffer can be used by writers, and by design that buffer cannot be destroyed until all its elements have been written and then read.

Alas! I guess we need something like https://www.justsoftwaresolutions.co.uk/threading/why-do-we-need-atomic_shared_ptr.html to keep the buffer alive while doing something with it. That will also make it possible to have thread-safe Pop()/PopAll(). Lock-free code is really complicated to get it right...
Comment on attachment 8902519 [details]
Bug 1394995 - Initial implementation of DecoderDoctorLogger -

https://reviewboard.mozilla.org/r/174110/#review187410

More to come...

::: dom/media/doctor/DDLifetime.h:37
(Diff revision 4)
> +  // Associated HTMLMediaElement, initially nullptr until this object can be
> +  // linked to its HTMLMediaElement.
> +  const dom::HTMLMediaElement* mMediaElement;
> +  // If not null, derived object for which this DDLifetime is a base class.
> +  // Note: We assume a single-inheritance hierarchy.
> +  DDLogObject mDerivedObject;

What's the purpose to store the log object of the derived class?

::: dom/media/doctor/DDLifetime.h:42
(Diff revision 4)
> +  DDLogObject mDerivedObject;
> +  DDMessageIndex mDerivedObjectLinkingIndex;
> +  // Negative and unique for unassociated objects.
> +  // Positive for associated objects, and unique for that HTMLMediaElement
> +  // group.
> +  int32_t mTag;

What is this tag for?

::: dom/media/doctor/DDLifetime.cpp:12
(Diff revision 4)
> +#include "DDLifetime.h"
> +
> +namespace mozilla {
> +
> +void
> +DDLifetime::AppendPrintf(nsCString& aString) const

Can you add comments to illustrate what this function will print?

::: dom/media/doctor/DDLifetime.cpp:20
(Diff revision 4)
> +    mObject.AppendPrintf(aString);
> +    aString.AppendPrintf("#%" PRIi32, mTag);
> +  } else if (mObject.mPointer == mDerivedObject.mPointer) {
> +    mDerivedObject.AppendPrintf(aString);
> +    aString.AppendPrintf("#%" PRIi32 " (as ", mTag);
> +    if (mObject.mPointer == mDerivedObject.mPointer) {

|mObject.mPointer == mDerivedObject.mPointer| is checked twice?

::: dom/media/doctor/DDLifetimes.h:34
(Diff revision 4)
> +                                 const DDMessageIndex& aIndex) const;
> +
> +  // Create a lifetime with the given object and construction index&time.
> +  DDLifetime& CreateLifetime(const DDLogObject& aObject,
> +                             DDMessageIndex aIndex,
> +                             const DDTimeStamp& aTimeStamp);

call this aConstructionTime?

::: dom/media/doctor/DDLifetimes.cpp:71
(Diff revision 4)
> +             aLifetime < lifetimes->Elements() + lifetimes->Length());
> +  DDL_LOG(aLifetime->mMediaElement ? mozilla::LogLevel::Debug
> +                                   : mozilla::LogLevel::Warning,
> +          "Remove lifetime %s",
> +          aLifetime->Printf().get());
> +  lifetimes->RemoveElementAt(aLifetime - lifetimes->Elements());

It is clearer to have a temp: |auto index = aLifetime - lifetimes->Elements()|.

Then you can assert |index >= 0 && index < Length()| and call RemoveElementAt(index).

::: dom/media/doctor/DDLogMessage.cpp:23
(Diff revision 4)
> +                   mIndex.Value(),
> +                   ToSeconds(mTimeStamp),
> +                   mObject.mTypeName,
> +                   mObject.mPointer,
> +                   ToShortString(mClass),
> +                   static_cast<const char*>(mLabel));

You don't need the casting.

::: dom/media/doctor/DDLogMessage.cpp:29
(Diff revision 4)
> +  AppendToString(mValue, str);
> +  return str;
> +}
> +
> +nsCString
> +DDLogMessage::Print(const DDLifetimes& aLifetimes) const

Can you illustrate what this function will print?

::: dom/media/doctor/DDLogObject.cpp:14
(Diff revision 4)
> +namespace mozilla {
> +
> +void
> +DDLogObject::AppendPrintf(nsCString& mString) const
> +{
> +  mString.AppendPrintf("%s[%p]", mTypeName ? mTypeName : "NULL", mPointer);

What does it mean when mTypeName is null?

::: dom/media/doctor/DDLogValue.h:25
(Diff revision 4)
> +
> +// Value capturing a range (typically an offset and a number of bytes in a
> +// resource).
> +struct DDRange
> +{
> +  int64_t mOffset;

const.

::: dom/media/doctor/DDLogValue.cpp:57
(Diff revision 4)
> +    mString.AppendPrintf("uint64_t(%" PRIu64 ")", a);
> +  }
> +  void match(double a) const { mString.AppendPrintf("double(%f)", a); }
> +  void match(const DDRange& a) const
> +  {
> +    mString.AppendPrintf("%" PRIi64 "<=(%" PRIi64 "B)<%" PRIi64 "",

Might be more readable to show [x, y).

::: dom/media/doctor/DDMediaLogs.h:23
(Diff revision 4)
> +struct DDMediaLogs
> +{
> +public:
> +  // Constructor ensures mMediaLogs[0] is associated with nullptr.
> +  // Out: rv reports the success/failure of creating the processing thread.
> +  explicit DDMediaLogs(nsresult* rv);

This pattern is weird. Please use the factory pattern.
Comment on attachment 8902519 [details]
Bug 1394995 - Initial implementation of DecoderDoctorLogger -

https://reviewboard.mozilla.org/r/174110/#review187424

::: dom/media/doctor/DDMediaLogs.h:71
(Diff revision 4)
> +    const dom::HTMLMediaElement* aMediaElement);
> +
> +  size_t SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const;
> +
> +private:
> +  DDMediaLog& LogFor(decltype(nullptr));

Can you document below functions for their usage and when they should be called.

::: dom/media/doctor/DDMediaLogs.h:115
(Diff revision 4)
> +
> +  DDLifetimes mLifetimes;
> +
> +  // mMediaLogs[0] contains unsorted message (with mMediaElement=nullptr).
> +  // mMediaLogs[1+] contains sorted messages for each media element.
> +  nsTArray<DDMediaLog> mMediaLogs;

Might be clearer to have another member for the unsorted messages.

::: dom/media/doctor/DDMediaLogs.cpp:26
(Diff revision 4)
> +
> +  *rv = NS_NewNamedThread("DDMediaLogs",
> +                          getter_AddRefs(mThread),
> +                          nullptr,
> +                          SharedThreadPool::kStackSize);
> +  DDL_INFO("DDMediaLogs constructed, processing thread: %p\n", mThread.get());

Is '\n' needed?

::: dom/media/doctor/DDMediaLogs.cpp:32
(Diff revision 4)
> +}
> +
> +DDMediaLogs::~DDMediaLogs()
> +{
> +  // Ensure the processing thread is shutdown.
> +  Shutdown(false);

|thread->Shutdown()| is the only thing to be done here. No need to call XXX.Clear() which will be handled by their destructors.

::: dom/media/doctor/DDMediaLogs.cpp:50
(Diff revision 4)
> +    DDL_INFO("DDMediaLogs::Shutdown will shutdown thread: %p", thread.get());
> +    thread->Shutdown();
> +  }
> +
> +  if (aPerformShutdownLogging &&
> +      MOZ_LOG_TEST(sDecoderDoctorLoggerEndLog, mozilla::LogLevel::Info)) {

This looks weird to me that the control flow depends on the logging level.

::: dom/media/doctor/DDMediaLogs.cpp:83
(Diff revision 4)
> +}
> +
> +DDMediaLog&
> +DDMediaLogs::LogFor(decltype(nullptr))
> +{
> +  MOZ_ASSERT(!mThread || mThread.get() == NS_GetCurrentThread());

We shouldn't call member functions if an object fails to init. We should assert |mThread.get() == NS_GetCurrentThread()| only.

::: dom/media/doctor/DDTimeStamp.cpp:13
(Diff revision 4)
> +
> +namespace mozilla {
> +
> +// Timestamp at start of process, used internally to convert log timestamps
> +// to a duration from this timestamp.
> +static const DDTimeStamp sInitialTimeStamp = TimeStamp::Now();

Move this into ToSeconds() for we don't want to increase launch time by using static initialization provided function static init is thread safe.
Thank you JW and Nathan for your reviews so far.
I'm going to push an update to RollingNumber and MultiWriterQueue, and then respond to comments about them.

JW, I'll work on DDLogger comments next.
Comment on attachment 8909186 [details]
Bug 1394995 - RollingNumber -

https://reviewboard.mozilla.org/r/180768/#review185854

> I would like to discuss the usage of RN.
> 
> E.g. for |r1 < r2|, will |r1/10 < r2/10| still hold?
> 
> To avoid such ambiguity, we can:
> 1. Provide a generator to control how a rolling number is generated.
> 2. Provide only comparison operators which are all we need to check the order of 2 RNs.
> 3. Do we need to convert a RN to its underlying integer type? When is this conversion useful?

You're right that allowing all arithmetic operations on RN doesn't make total sense.

So this update makes it more restrictive:
- Conversion from a number is explicit, so you can't use RN's "strange" comparisons by mistake.
- Only addition&subtractions are allowed, and they make most sense and are consistent with comparison operators.
The access through Value() is still there, it's mostly used for logging, but is also necessary when dealing with Atomic<RollingNumber::ValueType>.

I hope I've addressed your concerns.

> gtest is the trend, right?

I didn't notice the gtest directory in XPCOM! Anyway, I've moved everything to dom/media/doctor, and converted the tests to gtests.
Comment on attachment 8909186 [details]
Bug 1394995 - RollingNumber -

https://reviewboard.mozilla.org/r/180768/#review186048

I understand, I've moved it back to dom/media/doctor where it's needed.

As to the question of what range to handle for comparisons, half the type range seemed most natural, consistent with what would happen for signed numbers (i.e., if you translated a number by a comparison number, you would find that half the range is positive and the other half is negative -- not sure if that makes sense?)

Anyway, as it's media-only for now, I'll keep it as-is, and we can make it more generic later on, if others see a need for it.

> This would need to be added to `testing/cppunittest.ini` too.  MFBT also has gtests nowadays, so you could add it there instead.

Moved tests to dom/media/doctor/gtest.
Comment on attachment 8909187 [details]
Bug 1394995 - MultiWriterQueue -

https://reviewboard.mozilla.org/r/180770/#review185874

> |const F& aF|.

`const` is actually the wrong part. I want `F&&` to allow rvalues, as is usual when calling a function with a lambda.

> Might be worth supporting types that can't be default initialized as Maybe<T> does.

I'd prefer to keep it simple for now, especially because of the recycling where we end up re-using values (so why destroy objects when other may be re-created at the same spot?)
The workaround for now would be to use Maybe itself. :-)

It could still be done, of course; I'll leave that for another bug, when actually needed.

> Make mOlder and mStart const by removing in-class initialization.

Due to buffer recycling and other node management (adding/removing neighbors), I cannot make these const.

> How about calling it 'RecycleBuffer'?

Good idea, except it's confusing to know whether "Recycle" is the act of getting rid of something, or the later act of reusing something that was discarded! So I changed it to "StopUsing".
Also, now this function can delete buffers, so it's not always recycling.

> I am worried this might result in stack overflow if the thread observing |i == lastIndex| is stalled somehow.

I was hoping tail-calling would prevent that, but of course it's not the case in non-optimized builds.
So I've added a small while loop to wait for an actual change of value, plus a 'yield' to give other threads a change to run; in the worst case, the calling code will just feel the same effect as if it had been preemptively switched-away from.
Comment on attachment 8909187 [details]
Bug 1394995 - MultiWriterQueue -

https://reviewboard.mozilla.org/r/180770/#review186052

Moved back to dom/media/doctor.

I think lock-free data structures are not such an event anymore, especially since C++11, which introduced std::atomic and a proper memory model.

I'm afraid this particular algorithm is home-grown, as it's tailored for my particular use (multiple writers that I don't want to block, one non-critical reader in a single thread).

I've read&watched many lock-free articles/videos, so I'd like to think I've got some ideas of how to do this (Am I in the Dunning-Kruger sweet spot?), but I was hoping for better experts to help validate this implementation -- if you don't feel comfortable you can, do you know others who could help?
And I'll have a look around, see if there's such a thing already out there.

> Perhaps this restriction should be enforced in the BufferedElementAndNewBuffer class somehow?  I don't think you can enforce it completely, but a:
> 
> ```
>   template<typename F> Initialize(F&&);
>   template<typename F> Invalidate(F&&);
> ```
> 
> API, maybe with a `bool IsValid()` method, would go some way to centralizing the need to write `mValid` *after* accesses.

Good idea. I've revamped both Buffer and BufferedElement APIs to tighten their responsibilities:
- BufferedElement ensures read/writes are consistent with the internal validity flag.
- Buffer forwards read/writes to the appropriate BufferedElement.
- Outside functions NewBuffer and StopUsing deal with most of the management of buffer queues.

> It's not thread-safe with other `PopAll` calls, but pushes from other threads while we're popping things off are allowed?  That's...kind of unusual, isn't it?  How sure are we that such a model is safe?
> 
> I would feel better about this if there were some sign, like `const MutexAutoLock&` or something, that strongly encouraged the programmer to realize that he should provide his own guarantees around non-concurrent access here.  Mutex references are not ideal, because that suggests that you can't push to the queue during on-going deletions, but something along those lines.

The intended use is multiple writers safely pushing things, and then one reader popping them.
In particular, I view the reader as always being in one thread (or at least one taskqueue), so there's no need for an actual mutex. We do this a lot in media at least.

But I understand that at a glance, it may be confusing to have this duality of some functions being thread-safe and others not.
I could in fact go with a `const MutexAutoLock&` as you suggest, as though in my use case it would be unneeded, it's a fairly low overhead for the peace of mind.
Would you see another way to deal with this?

> It seems peculiar to not advise users of this (significant!) downside in the public documentation, or provide some way of addressing it through the public interface.  This is a design just asking to cause memory usage issues.
> 
> The last sentence is also inaccurate: "users should ensure queues are regularly popped, to avoid hanging on too (sic) much memory between peaks."  Even if we're popping regularly, we're not actually releasing memory, we're just moving it to the freelist.  So our memory usage is always proportional to the high-water usage of the queue, regardless of the particular sequence of actions taken on the queue.  This is Not Good.

I've now implemented a simple heuristics of dropping every second buffer during a multi-buffer read. I'm sure there could be better algorithms, but this one is simple and it does the job of reducing memory usage. I've added a TODO as a reminder to revisit.

Popping regularly does help, as it moves reusable buffers to the freelist, in which case later pushes won't need to allocate brand-new buffers, it keeps the watermark lower than it would otherwise be, and also reduces the costs of allocating & freeing buffers.

> Introducing an ostensibly thread-safe class whose tests don't feature threads in any way seems like very brave programming.  Putting this in a framework that accommodates threads, like gtests, would probably be better, and then rewriting things so we had some kind of multi-threaded access going on would be even better.

Thank you! (I think)
My main testing was actually done through using it in a real-world example of logging many thousands of log messages per second (next patch).
Also I didn't notice the MFBT had a gtest directory (hidden under mfbt/tests), that's why I didn't write one to start with.

But I've now added one, which I think exercizes the code quite well, it even proved a race I hypothesized (see comment 34, now fixed) so it's definitely valuable.
Comment on attachment 8909187 [details]
Bug 1394995 - MultiWriterQueue -

https://reviewboard.mozilla.org/r/180770/#review186052

> The intended use is multiple writers safely pushing things, and then one reader popping them.
> In particular, I view the reader as always being in one thread (or at least one taskqueue), so there's no need for an actual mutex. We do this a lot in media at least.
> 
> But I understand that at a glance, it may be confusing to have this duality of some functions being thread-safe and others not.
> I could in fact go with a `const MutexAutoLock&` as you suggest, as though in my use case it would be unneeded, it's a fairly low overhead for the peace of mind.
> Would you see another way to deal with this?

Alright, to avoid the potential confusion between multi-threaded writing vs single-threaded reading, I've now made reading thread-safe by default (so it's consistent with writing); But with a non-locking option if requested, in which case DEBUG-mode assertions should catch misuses.
What do you think?
Comment on attachment 8909186 [details]
Bug 1394995 - RollingNumber -

If JW is happy with this, then I don't think my review is needed, given that this has now moved into media code.
Attachment #8909186 - Flags: review?(nfroyd)
Comment on attachment 8902519 [details]
Bug 1394995 - Initial implementation of DecoderDoctorLogger -

https://reviewboard.mozilla.org/r/174110/#review187410

> What's the purpose to store the log object of the derived class?

In summary, it's a kind of linking that can associate two (potentially-different) pointers of (seemingly) different types that actually point to the same object, so that logs may be associated with their concrete object. (I'll add a comment.)

The logging functions only have access to the type of `this`, so in methods of a base class, they cannot directly know which concrete object they are working on. So they just record the pointer value and the known class name.
During construction of a derived object, a "construction-with-base" log message is stored, that contains the pointer value, and the names of both the actual class and the base class.

Later, during log processing, the special construction-with-base is used to link pointers to both the object and to its base class, which is used to gather logs from linked objects.
This relationship is also stored in a DDLifetime, which is used to more easily show this relationship when outputting logs. E.g.:
> dom::HTMLVideoElement[134073800]#1 (as dom::HTMLMediaElement) | lnk | decoder | MediaSourceDecoder[136078200]#5 (as MediaDecoder)
This log originally came from an HTMLMediaElement method, but we can helpfully show that in fact the object was an HTMLVideoElement; and same with the MediaDecoder/MediaSourceDecoder object.

The reason we store the derived object (instead of the derived object storing its base) is visible in the above log example: A log line may originate from a base class method, but we want to show its real object, which is the most-derived object.
This could be reversed if really needed, but it would make things more complex, so I'd prefer to keep it that way for now.

> What is this tag for?

This is a unique tag used to identify objects in each log, much more readable than an object pointer. (I'll add a comment.)

> |mObject.mPointer == mDerivedObject.mPointer| is checked twice?

The first `if` (after `else`) was wrong! Good catch.

> What does it mean when mTypeName is null?

It should only happen if the object was printed before it is set; but this should never actually happen!
I will revamp DDLogObject to better prevent misuse.

> Might be more readable to show [x, y).

I wanted to also display the range length, as it may be useful in some contexts, e.g.: "2289<=(2759B)<5048".
This output is intended for humans, so they don't have to manually calculate such trivially-computed numbers. ;-)
Comment on attachment 8902519 [details]
Bug 1394995 - Initial implementation of DecoderDoctorLogger -

https://reviewboard.mozilla.org/r/174110/#review187424

> Might be clearer to have another member for the unsorted messages.

It would, but it would also make other things harder, e.g. loops over *all* mMediaLogs (including [0]), for which I'd need to duplicate or refactor code.

> |thread->Shutdown()| is the only thing to be done here. No need to call XXX.Clear() which will be handled by their destructors.

True.
I've reworked the shutdown procedures:
- Normal shutdown (with processing) will happen on ~DDMediaLogs() only.
- Abnormal shutdown (no processing, but freeing memory) will happen when DDMediaLogs::Panic() is called.

> This looks weird to me that the control flow depends on the logging level.

That whole block only performs some processing to output to MOZ_LOG=DDLoggerEnd, so there's no point doing any of it if that log is not enabled. I'll add a comment to explain.

> We shouldn't call member functions if an object fails to init. We should assert |mThread.get() == NS_GetCurrentThread()| only.

These processing functions may also be called *after* normal shutdown, from outside of the (now-destroyed) processing thread.
See Shutdown(false) doing: thread->Shutdown, mThread=0, ProcessLog.

> Move this into ToSeconds() for we don't want to increase launch time by using static initialization provided function static init is thread safe.

Initial thoughts:
- We may get negative seconds, because we would use the first call of ToSeconds() as initial timestamp, to compare with timestamp that were recorded *before*.
- Also I think we would suffer the cost of this thread-safe initialize-or-test at *every* call of ToSeconds().

But then... Timestamps are relative anyway, so it's fine (if a bit strange) to have negative times. And ToSeconds is only used during processing in a background thread, so it's fine if it's a tiny bit slower.
So I'll make the change.
Comment on attachment 8909186 [details]
Bug 1394995 - RollingNumber -

https://reviewboard.mozilla.org/r/180768/#review190030

::: dom/media/doctor/RollingNumber.h:72
(Diff revision 4)
> +    return *this;
> +  }
> +
> +  RollingNumber operator--(int) { return RollingNumber{ mIndex-- }; }
> +
> +  RollingNumber operator+(ValueType aIncrement) const

This can be implemented by operator+= to reduce the chance of inconsistency.

::: dom/media/doctor/RollingNumber.h:107
(Diff revision 4)
> +  // Distance between numbers.
> +
> +  ValueType operator-(RollingNumber aOther) const
> +  {
> +    // Don't allow differences of more than half the type range.
> +    MOZ_ASSERT(Value() - aOther.Value() <= MidWay);

I think we should assert
MOZ_ASSERT(aOther.Value() <= MidWay) to be consistent with operator-(ValueType aDecrement).

::: dom/media/doctor/RollingNumber.h:113
(Diff revision 4)
> +    return Value() - aOther.Value();
> +  }
> +
> +  // Normal (in)equality operators.
> +
> +  bool operator==(RollingNumber aOther) const

The convention is to pass |const T&| for comparison operators.

::: dom/media/doctor/RollingNumber.h:119
(Diff revision 4)
> +  {
> +    return Value() == aOther.Value();
> +  }
> +  bool operator!=(RollingNumber aOther) const
> +  {
> +    return Value() != aOther.Value();

This should be implemented by operator==.

::: dom/media/doctor/RollingNumber.h:126
(Diff revision 4)
> +
> +  // Modified comparison operators.
> +
> +  bool operator<(RollingNumber aOther) const
> +  {
> +    return ValueType(Value() - aOther.Value()) > MidWay;

1. Why explicit casting is needed?

::: dom/media/doctor/gtest/TestRollingNumber.cpp:28
(Diff revision 4)
> +  const RN8 n;
> +  // Access through Value().
> +  EXPECT_EQ(0, n.Value());
> +  // Implicit conversion constructor: 0 -> RollingNumber{0} as needed by
> +  // operator==.
> +  EXPECT_EQ(0, n.Value());

This is the same as #25.
Comment on attachment 8909186 [details]
Bug 1394995 - RollingNumber -

https://reviewboard.mozilla.org/r/180768/#review190030

> I think we should assert
> MOZ_ASSERT(aOther.Value() <= MidWay) to be consistent with operator-(ValueType aDecrement).

They have a different meaning (similar to the difference between `pointer - value` and `pointer - pointer`), so the consistency I'm trying to maintain is at a higher level:
The former is a translation of one RollingNumber by a number, and we don't want to move the RollingNumber too far.
The latter is a distance between two RollingNumbers which results in a number, and we don't allow a too-large distance.

> 1. Why explicit casting is needed?

Because of possible integer promotion:
http://en.cppreference.com/w/cpp/language/implicit_conversion#Integral_promotion
(I did try without the casts, and half the gtest failed!)
I'll make the casts even more visible, and add a comment about that.

And this made me realize that the check in `operator-(RollingNumber)` was missing that necessary cast.
So thank you for asking!

> This is the same as #25.

Oh, right, must be a relic from the previous version; and the comment is wrong anyway, so I'll remove it.
Comment on attachment 8909186 [details]
Bug 1394995 - RollingNumber -

https://reviewboard.mozilla.org/r/180768/#review190384

::: dom/media/doctor/RollingNumber.h:123
(Diff revision 5)
> +    return !(*this == aOther);
> +  }
> +
> +  // Modified comparison operators.
> +
> +  bool operator<(const RollingNumber& aOther) const

It is interesting that:

using RN8 = mozilla::RollingNumber<uint8_t>;
RN8 i{ 0 };
RN8 j{ 128 };

i<j tests to be true while
i<=j tests to be false.

We should assert 2 numbers for comparison shouldn't be too far away.
Attachment #8909186 - Flags: review?(jwwang) → review+
Comment on attachment 8902519 [details]
Bug 1394995 - Initial implementation of DecoderDoctorLogger -

https://reviewboard.mozilla.org/r/174110/#review191690

A huge patch. Hope I didin't miss any important details. :)

::: dom/media/doctor/DDLifetimes.cpp:44
(Diff revision 8)
> +  }
> +  return nullptr;
> +}
> +
> +DDLifetime&
> +DDLifetimes::CreateLifetime(const DDLogObject& aObject,

Should this method be thread-safe?

::: dom/media/doctor/DDLifetimes.cpp:55
(Diff revision 8)
> +  if (--sTag > 0) {
> +    sTag = -1;
> +  }
> +  LifetimesForObject* lifetimes = mLifetimes.LookupOrAdd(aObject, 1);
> +  DDLifetime& lifetime = *lifetimes->AppendElement(
> +    DDLifetime(aObject, aIndex, aConstructionTimeStamp, sTag));

Do we allow sTag==0 here?

::: dom/media/doctor/DDLogObject.h:40
(Diff revision 8)
> +
> +  void Set(const char* aTypeName, const void* aPointer)
> +  {
> +    MOZ_ASSERT(aTypeName);
> +    mTypeName = aTypeName;
> +    mPointer = aPointer;

Is it possible for aPointer to be null?

::: dom/media/doctor/DDMediaLogs.cpp:35
(Diff revision 8)
> +DDMediaLogs::DDMediaLogs(nsCOMPtr<nsIThread>&& aThread)
> +  : mMediaLogs(1)
> +  , mMutex("DDMediaLogs")
> +  , mThread(Move(aThread))
> +{
> +  mMediaLogs.SetLength(1);

How is it different from |mMediaLogs(1)| above?

::: dom/media/doctor/DDMediaLogs.cpp:36
(Diff revision 8)
> +  : mMediaLogs(1)
> +  , mMutex("DDMediaLogs")
> +  , mThread(Move(aThread))
> +{
> +  mMediaLogs.SetLength(1);
> +  mMediaLogs[0].mMediaElement = 0;

use nullptr?

::: dom/media/doctor/DDMediaLogs.cpp:106
(Diff revision 8)
> +    }
> +  }
> +}
> +
> +DDMediaLog&
> +DDMediaLogs::LogFor(decltype(nullptr))

It looks weird to call LogFor(nullptr) to get the unsorted logs. However about having a "UnsortedLog()" method?

::: dom/media/doctor/DDMediaLogs.cpp:156
(Diff revision 8)
> +
> +  // List of lifetimes that are to be linked to aMediaElement.
> +  nsTArray<DDLifetime*> lifetimes;
> +  // We start with the given lifetime.
> +  lifetimes.AppendElement(&aLifetime);
> +  for (size_t i = 0; i < lifetimes.Length(); ++i) {

This loop will only run once, right?

::: dom/media/doctor/DecoderDoctorLogger.cpp:127
(Diff revision 8)
> +      // Someone else changed the state.
> +      return EnsureLogIsEnabled();
> +    case scEnabled:
> +      return true;
> +    case scEnabling:
> +      return EnsureLogIsEnabled();

It feels scary to spin the stack. Can we just return false here at the cost of losing some logs?
Attachment #8902519 - Flags: review?(jwwang) → review+
Comment on attachment 8909186 [details]
Bug 1394995 - RollingNumber -

https://reviewboard.mozilla.org/r/180768/#review190384

> It is interesting that:
> 
> using RN8 = mozilla::RollingNumber<uint8_t>;
> RN8 i{ 0 };
> RN8 j{ 128 };
> 
> i<j tests to be true while
> i<=j tests to be false.
> 
> We should assert 2 numbers for comparison shouldn't be too far away.

My first reaction was that we should allow the full half-range, just because we can...

But it's true that I'm in fact intending RollingNumbers to always be used with "nearby" numbers.

So I'll add asserts to limit differences to a quarter of the range (which hopefully should be small enough to catch misuses that are most at risk of going all the way to overflow and therefore meaninglessness.)
And I'll add a comment at the top of the class to explain this.
Comment on attachment 8902519 [details]
Bug 1394995 - Initial implementation of DecoderDoctorLogger -

https://reviewboard.mozilla.org/r/174110/#review191690

Thank you very much for your time spent on this review.

If that's any comfort: All this code should only ever be used when a user chooses to run the Media DevTools, so if you missed anything, the impact should be minimal.

This last update also contains a printf-format checker (because the nsPrintfCString checker doesn't work when called from a templated variadic function), which found one misuse that was causing some test failures.

> Should this method be thread-safe?

No need, DDLifetimes is only used from MediaLogs::ProcessBuffer, which only runs in one thread.
I'll see if I can add something to both document this and catch misuses, probably something like MultiWriterQueueReaderLocking_None (see previous patch).

> Do we allow sTag==0 here?

Not sure why you ask this question.

It actually doesn't really matter: Tags are just human-friendly numbers to help users distinguish different objects.
By design (see above) this function should only give strictly-negative numbers to yet-unclassified objects; later on, DDMediaLogs::SetMediaElement will change that tag to a strictly-positive number.
So in effect I don't ever expect sTag==0; but it wouldn't be a real issue if that somehow happened.

Does that address any concern you may have had about this?

> Is it possible for aPointer to be null?

In practice, no, as it should come from a `this` pointer. I'll add a `MOZ_ASSERT(aPointer)`.

> How is it different from |mMediaLogs(1)| above?

mMediaLogs(1) sets the initial buffer *capacity* but keeps the actual length at zero elements. I also originally thought it would set the length, and was surprised when mMediaLogs[0] crashed! :-)

> It looks weird to call LogFor(nullptr) to get the unsorted logs. However about having a "UnsortedLog()" method?

Fair enough. I'll go with LogForUnassociatedMessages() for maximum clarity and correctness.

> This loop will only run once, right?

`lifetimes` is a growing list of DDLifetime objects to examine.
It will start with 1 element, but if you look below, I do `lifetimes.AppendElement()`, so if a loop finds a new linked element, it will be added to be processed in a later loop.
And I do need to keep all already-processed lifetimes, so I don't process them again.

> It feels scary to spin the stack. Can we just return false here at the cost of losing some logs?

You're right, I always assume that a tail call will result in a "goto", but at least it's not true in non-optimized builds.
I'll change the whole thing into an endless for loop.
Comment on attachment 8909186 [details]
Bug 1394995 - RollingNumber -

https://reviewboard.mozilla.org/r/180768/#review192120


C/C++ static analysis found 6 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/doctor/gtest/TestRollingNumber.cpp:17
(Diff revision 6)
> +#include <cstdint>
> +#include <gtest/gtest.h>
> +
> +using RN8 = mozilla::RollingNumber<uint8_t>;
> +
> +TEST(RollingNumber, Value)

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: dom/media/doctor/gtest/TestRollingNumber.cpp:17
(Diff revision 6)
> +#include <cstdint>
> +#include <gtest/gtest.h>
> +
> +using RN8 = mozilla::RollingNumber<uint8_t>;
> +
> +TEST(RollingNumber, Value)

Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]

::: dom/media/doctor/gtest/TestRollingNumber.cpp:44
(Diff revision 6)
> +  // Assignment.
> +  n42 = n;
> +  EXPECT_EQ(0, n42.Value());
> +}
> +
> +TEST(RollingNumber, Operations)

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: dom/media/doctor/gtest/TestRollingNumber.cpp:44
(Diff revision 6)
> +  // Assignment.
> +  n42 = n;
> +  EXPECT_EQ(0, n42.Value());
> +}
> +
> +TEST(RollingNumber, Operations)

Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]

::: dom/media/doctor/gtest/TestRollingNumber.cpp:95
(Diff revision 6)
> +  EXPECT_EQ(246, n.Value());
> +  n += 20;
> +  EXPECT_EQ(10, n.Value());
> +}
> +
> +TEST(RollingNumber, Comparisons)

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: dom/media/doctor/gtest/TestRollingNumber.cpp:95
(Diff revision 6)
> +  EXPECT_EQ(246, n.Value());
> +  n += 20;
> +  EXPECT_EQ(10, n.Value());
> +}
> +
> +TEST(RollingNumber, Comparisons)

Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]
Comment on attachment 8909187 [details]
Bug 1394995 - MultiWriterQueue -

https://reviewboard.mozilla.org/r/180770/#review192122


C/C++ static analysis found 8 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/doctor/gtest/TestMultiWriterQueue.cpp:50
(Diff revision 6)
> +    // Nothing left.
> +    q.PopAll([&](int&) { EXPECT_TRUE(false); });
> +  }
> +}
> +
> +TEST(MultiWriterQueue, SingleThreaded)

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: dom/media/doctor/gtest/TestMultiWriterQueue.cpp:50
(Diff revision 6)
> +    // Nothing left.
> +    q.PopAll([&](int&) { EXPECT_TRUE(false); });
> +  }
> +}
> +
> +TEST(MultiWriterQueue, SingleThreaded)

Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]

::: dom/media/doctor/gtest/TestMultiWriterQueue.cpp:172
(Diff revision 6)
> +         q.ReusableBuffersStats().mWatermark,
> +         q.AllocatedBuffersStats().mCount,
> +         q.AllocatedBuffersStats().mWatermark);
> +}
> +
> +TEST(MultiWriterQueue, MultiWriterSingleReader)

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: dom/media/doctor/gtest/TestMultiWriterQueue.cpp:172
(Diff revision 6)
> +         q.ReusableBuffersStats().mWatermark,
> +         q.AllocatedBuffersStats().mCount,
> +         q.AllocatedBuffersStats().mWatermark);
> +}
> +
> +TEST(MultiWriterQueue, MultiWriterSingleReader)

Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]

::: dom/media/doctor/gtest/TestMultiWriterQueue.cpp:233
(Diff revision 6)
> +  // with no locking) crash; uncomment to verify.
> +  // TestMultiWriterQueueMT<
> +  //   MultiWriterQueue<int, MultiWriterQueueDefaultBufferSize, MultiWriterQueueReaderLocking_None>>(64, 2, 2*1024*1024);
> +}
> +
> +TEST(MultiWriterQueue, MultiWriterMultiReader)

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: dom/media/doctor/gtest/TestMultiWriterQueue.cpp:233
(Diff revision 6)
> +  // with no locking) crash; uncomment to verify.
> +  // TestMultiWriterQueueMT<
> +  //   MultiWriterQueue<int, MultiWriterQueueDefaultBufferSize, MultiWriterQueueReaderLocking_None>>(64, 2, 2*1024*1024);
> +}
> +
> +TEST(MultiWriterQueue, MultiWriterMultiReader)

Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]

::: dom/media/doctor/gtest/TestMultiWriterQueue.cpp:378
(Diff revision 6)
> +      aF(i);
> +    }
> +  }
> +};
> +
> +TEST(MultiWriterQueue, nsDequeBenchmark)

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: dom/media/doctor/gtest/TestMultiWriterQueue.cpp:378
(Diff revision 6)
> +      aF(i);
> +    }
> +  }
> +};
> +
> +TEST(MultiWriterQueue, nsDequeBenchmark)

Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]
Comment on attachment 8902519 [details]
Bug 1394995 - Initial implementation of DecoderDoctorLogger -

https://reviewboard.mozilla.org/r/174110/#review192124


C/C++ static analysis found 6 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/doctor/DDLogValue.cpp:19
(Diff revision 9)
> +{
> +  nsCString& mString;
> +
> +  void match(const DDNoValue&) const {}
> +  void match(const DDLogObject& a) const { a.AppendPrintf(mString); }
> +  void match(const char* a) const { mString.AppendPrintf("\"%s\"", a); }

Warning: Escaped string literal can be written as a raw string literal [clang-tidy: modernize-raw-string-literal]

  void match(const char* a) const { mString.AppendPrintf("\"%s\"", a); }
                                                         ^
                                                         R"("%s")"

::: dom/media/doctor/DDLogValue.cpp:22
(Diff revision 9)
> +  void match(const DDNoValue&) const {}
> +  void match(const DDLogObject& a) const { a.AppendPrintf(mString); }
> +  void match(const char* a) const { mString.AppendPrintf("\"%s\"", a); }
> +  void match(const nsCString& a) const
> +  {
> +    mString.AppendPrintf("nsCString(\"%s\")", a.Data());

Warning: Escaped string literal can be written as a raw string literal [clang-tidy: modernize-raw-string-literal]

    mString.AppendPrintf("nsCString(\"%s\")", a.Data());
                         ^
                         R"(nsCString("%s"))"

::: dom/media/doctor/DDLogValue.cpp:73
(Diff revision 9)
> +  }
> +  void match(const MediaResult& a) const
> +  {
> +    nsCString name;
> +    GetErrorName(a.Code(), name);
> +    mString.AppendPrintf("MediaResult(%s =0x%08" PRIx32 ", \"%s\")",

Warning: Escaped string literal can be written as a raw string literal [clang-tidy: modernize-raw-string-literal]

    mString.AppendPrintf("MediaResult(%s =0x%08" PRIx32 ", \"%s\")",
                         ^
                         R"(MediaResult(%s =0x%08x, "%s"))"

::: dom/media/doctor/DDLogValue.cpp:96
(Diff revision 9)
> +  void match(const DDNoValue&) const { mJW.NullProperty(mPropertyName); }
> +  void match(const DDLogObject& a) const
> +  {
> +    mJW.StringProperty(
> +      mPropertyName,
> +      nsPrintfCString("\"%s[%p]\"", a.TypeName(), a.Pointer()).get());

Warning: Escaped string literal can be written as a raw string literal [clang-tidy: modernize-raw-string-literal]

      nsPrintfCString("\"%s[%p]\"", a.TypeName(), a.Pointer()).get());
                      ^
                      R"("%s[%p]")"

::: dom/media/doctor/DDLogValue.cpp:132
(Diff revision 9)
> +  {
> +    nsCString name;
> +    GetErrorName(a.Code(), name);
> +    mJW.StringProperty(
> +      mPropertyName,
> +      nsPrintfCString("\"MediaResult(%s, %s)\"", name.get(), a.Message().get())

Warning: Escaped string literal can be written as a raw string literal [clang-tidy: modernize-raw-string-literal]

      nsPrintfCString("\"MediaResult(%s, %s)\"", name.get(), a.Message().get())
                      ^
                      R"lit("MediaResult(%s, %s)")lit"


::: dom/media/doctor/DDMediaLogs.cpp:474
(Diff revision 9)
> +        mLifetimes.FindLifetime(message.mObject, message.mIndex);
> +      if (lifetime) {
> +        jw.IntProperty("ob", lifetime->mTag);
> +      } else {
> +        jw.StringProperty("ob",
> +                          nsPrintfCString("\"%s[%p]\"",

Warning: Escaped string literal can be written as a raw string literal [clang-tidy: modernize-raw-string-literal]

                          nsPrintfCString("\"%s[%p]\"",
                                          ^
                                          R"("%s[%p]")"
This last review-push was just changing string literals as suggested in comment 75, and running clang-format again.

Will land shortly, yay!

Thank you again JW for this long review.
The next one should only be half as long, but much simpler. ;-)
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6947cddff40c
Moved DecoderDoctor files to dom/media/doctor/ - r=jwwang
https://hg.mozilla.org/integration/autoland/rev/829e37a5150f
RollingNumber - r=jwwang
https://hg.mozilla.org/integration/autoland/rev/dac1ee688e1e
MultiWriterQueue - r=jwwang
https://hg.mozilla.org/integration/autoland/rev/fc8bdc8baa65
Initial implementation of DecoderDoctorLogger - r=jwwang
Comment on attachment 8909186 [details]
Bug 1394995 - RollingNumber -

https://reviewboard.mozilla.org/r/180768/#review192568


C/C++ static analysis found 6 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/doctor/gtest/TestRollingNumber.cpp:17
(Diff revision 7)
> +#include <cstdint>
> +#include <gtest/gtest.h>
> +
> +using RN8 = mozilla::RollingNumber<uint8_t>;
> +
> +TEST(RollingNumber, Value)

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: dom/media/doctor/gtest/TestRollingNumber.cpp:17
(Diff revision 7)
> +#include <cstdint>
> +#include <gtest/gtest.h>
> +
> +using RN8 = mozilla::RollingNumber<uint8_t>;
> +
> +TEST(RollingNumber, Value)

Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]

::: dom/media/doctor/gtest/TestRollingNumber.cpp:44
(Diff revision 7)
> +  // Assignment.
> +  n42 = n;
> +  EXPECT_EQ(0, n42.Value());
> +}
> +
> +TEST(RollingNumber, Operations)

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: dom/media/doctor/gtest/TestRollingNumber.cpp:44
(Diff revision 7)
> +  // Assignment.
> +  n42 = n;
> +  EXPECT_EQ(0, n42.Value());
> +}
> +
> +TEST(RollingNumber, Operations)

Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]

::: dom/media/doctor/gtest/TestRollingNumber.cpp:95
(Diff revision 7)
> +  EXPECT_EQ(246, n.Value());
> +  n += 20;
> +  EXPECT_EQ(10, n.Value());
> +}
> +
> +TEST(RollingNumber, Comparisons)

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: dom/media/doctor/gtest/TestRollingNumber.cpp:95
(Diff revision 7)
> +  EXPECT_EQ(246, n.Value());
> +  n += 20;
> +  EXPECT_EQ(10, n.Value());
> +}
> +
> +TEST(RollingNumber, Comparisons)

Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]
Comment on attachment 8909187 [details]
Bug 1394995 - MultiWriterQueue -

https://reviewboard.mozilla.org/r/180770/#review192570


C/C++ static analysis found 8 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/doctor/gtest/TestMultiWriterQueue.cpp:50
(Diff revision 7)
> +    // Nothing left.
> +    q.PopAll([&](int&) { EXPECT_TRUE(false); });
> +  }
> +}
> +
> +TEST(MultiWriterQueue, SingleThreaded)

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: dom/media/doctor/gtest/TestMultiWriterQueue.cpp:50
(Diff revision 7)
> +    // Nothing left.
> +    q.PopAll([&](int&) { EXPECT_TRUE(false); });
> +  }
> +}
> +
> +TEST(MultiWriterQueue, SingleThreaded)

Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]

::: dom/media/doctor/gtest/TestMultiWriterQueue.cpp:172
(Diff revision 7)
> +         q.ReusableBuffersStats().mWatermark,
> +         q.AllocatedBuffersStats().mCount,
> +         q.AllocatedBuffersStats().mWatermark);
> +}
> +
> +TEST(MultiWriterQueue, MultiWriterSingleReader)

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: dom/media/doctor/gtest/TestMultiWriterQueue.cpp:172
(Diff revision 7)
> +         q.ReusableBuffersStats().mWatermark,
> +         q.AllocatedBuffersStats().mCount,
> +         q.AllocatedBuffersStats().mWatermark);
> +}
> +
> +TEST(MultiWriterQueue, MultiWriterSingleReader)

Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]

::: dom/media/doctor/gtest/TestMultiWriterQueue.cpp:233
(Diff revision 7)
> +  // with no locking) crash; uncomment to verify.
> +  // TestMultiWriterQueueMT<
> +  //   MultiWriterQueue<int, MultiWriterQueueDefaultBufferSize, MultiWriterQueueReaderLocking_None>>(64, 2, 2*1024*1024);
> +}
> +
> +TEST(MultiWriterQueue, MultiWriterMultiReader)

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: dom/media/doctor/gtest/TestMultiWriterQueue.cpp:233
(Diff revision 7)
> +  // with no locking) crash; uncomment to verify.
> +  // TestMultiWriterQueueMT<
> +  //   MultiWriterQueue<int, MultiWriterQueueDefaultBufferSize, MultiWriterQueueReaderLocking_None>>(64, 2, 2*1024*1024);
> +}
> +
> +TEST(MultiWriterQueue, MultiWriterMultiReader)

Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]

::: dom/media/doctor/gtest/TestMultiWriterQueue.cpp:378
(Diff revision 7)
> +      aF(i);
> +    }
> +  }
> +};
> +
> +TEST(MultiWriterQueue, nsDequeBenchmark)

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: dom/media/doctor/gtest/TestMultiWriterQueue.cpp:378
(Diff revision 7)
> +      aF(i);
> +    }
> +  }
> +};
> +
> +TEST(MultiWriterQueue, nsDequeBenchmark)

Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]
Comment on attachment 8902519 [details]
Bug 1394995 - Initial implementation of DecoderDoctorLogger -

https://reviewboard.mozilla.org/r/174110/#review192572


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/doctor/DDLogValue.cpp:73
(Diff revision 10)
> +  }
> +  void match(const MediaResult& a) const
> +  {
> +    nsCString name;
> +    GetErrorName(a.Code(), name);
> +    mString.AppendPrintf("MediaResult(%s =0x%08" PRIx32 ", \"%s\")",

Warning: Escaped string literal can be written as a raw string literal [clang-tidy: modernize-raw-string-literal]

    mString.AppendPrintf("MediaResult(%s =0x%08" PRIx32 ", \"%s\")",
                         ^
                         R"(MediaResult(%s =0x%08x, "%s"))"
Depends on: 1406980
You need to log in before you can comment on or make changes to this bug.