Closed Bug 970691 Opened 10 years ago Closed 10 years ago

Add timestamp to fake video

Categories

(Core :: WebRTC: Audio/Video, defect)

Product:
Core
Component:
WebRTC: Audio/Video
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla31

People

(Reporter: abr, Assigned: pkerr)

References

Details

(Whiteboard: [p=3, 1.5:p1, ft:webrtc])

Attachments

(2 files, 11 obsolete files)

Part 1: Add timestamp to fake video
18.00 KB, patch
Details | Diff | Splinter Review
Part2: Restore digit stamping function to YuvStamper
13.78 KB, patch
Details | Diff | Splinter Review 
We need timestamp information embedded in the fake video stream to facilitate measurement of video latency
Blocks: 970426
I suggest a QR code with frame number and time
I suspect there are JS libs for parsing a QR code from video in a canvas (or creating one)

If not, some simple binary encoding in white/black blocks in a row perhaps
I can produce a patch for this, or if you want to have someone else make a patch.
Assignee: nobody → ekr
Status: NEW → ASSIGNED
Comment on attachment 8395292 [details] [diff] [review]
Stamp each frame with a timestamp.

Review of attachment 8395292 [details] [diff] [review]:
-----------------------------------------------------------------

Here is an incomplete patch with the following functionality:

1. Encode an arbitrary bitstring into a YUV frame.
2. Decode a bitstring out of a YUV frame
3. Encode numbers into a YUV frame so they are visible as
part of a heads-up display.
4. Encode the timestamp into every fake video frame
for getUserMedia({fake:true}).

Note that while I have tested the bitstring decoding in another
branch, no code in this patch uses it.

Also, note that the code to export YuvStamper.h is a hack. Maybe
there is something better.

Finally, this code has only been run in prototype code, so needs
review, etc.
Paul -- Do you think you could finish off this work that Ekr started and land it?
Assignee: ekr → paulrkerr
(In reply to Maire Reavy [:mreavy] (Please needinfo me if you need my attention on a bug) from comment #7)
> Paul -- Do you think you could finish off this work that Ekr started and
> land it?

Yes, I can do this. Can you give me an idea of priority or due date?
Flags: needinfo?(mreavy)
I want to get this into Gecko 31 (which uplifts April 28th), but it may make sense to do this now or very soon -- since Bug 970696 will likely want to leverage this for video latency measurements.  If you and Randell could coordinate directly, that would be ideal.  (I want to get as many dependent bugs of Bug 970426 into Gecko 31 as we can.)  Thanks.
Flags: needinfo?(mreavy)
How do you want to receive the extracted and decoded timestamp from a received fake video stream? The MediaEngineDefaultVideoSource with ekr's patch inserts the encoded timestamp in each frame (there is also code that would embed digits). I presume you don't want the timestamp to just be visible in the image but provide an extraction path in the graph somewhere for instrumentation. Do you have a mechanism in mind?
Flags: needinfo?(adam)
If ekr's code doesn't have an obvious way to extract it (from a lossy image perhaps of lower resolution), we may need to use something else (like QR codes above) in comment 2
(In reply to Randell Jesup [:jesup] from comment #11)
> If ekr's code doesn't have an obvious way to extract it (from a lossy image
> perhaps of lower resolution), we may need to use something else (like QR
> codes above) in comment 2

There is a method that can extract the encoded data. What I want to know is where would that method be called on the receiving end. For example, if you use the WebRTC Test Landing Page with this patch applied you can see the encoding in the colored frames. What is the equivalent to MediaEngineDefaultVideoSource on the sink side that would extract the timestamps. In ekr's example his pulls data out in WebrtcVideoConduit::DeliverFrame but then uses the YuvStamper to write the received data back into the image. For latency testing, do we want to provide decoded numeric associated with each frame after extracting from the visual data received?
(In reply to Paul Kerr [:pkerr] from comment #12)
> (In reply to Randell Jesup [:jesup] from comment #11)
> > If ekr's code doesn't have an obvious way to extract it (from a lossy image
> > perhaps of lower resolution), we may need to use something else (like QR
> > codes above) in comment 2
> 
> There is a method that can extract the encoded data.

Indeed. And I use it regularly so it works fine :)


> What I want to know is
> where would that method be called on the receiving end. For example, if you
> use the WebRTC Test Landing Page with this patch applied you can see the
> encoding in the colored frames. What is the equivalent to
> MediaEngineDefaultVideoSource on the sink side that would extract the
> timestamps. In ekr's example his pulls data out in
> WebrtcVideoConduit::DeliverFrame but then uses the YuvStamper to write the
> received data back into the image. For latency testing, do we want to
> provide decoded numeric associated with each frame after extracting from the
> visual data received?
Per IRC conversation, I believe the conclusion here is that we're going to pull the timestamp data out and put it in the stats object.
Flags: needinfo?(adam)
Work in progress: http://hg.mozilla.org/users/paulrkerr_gmail.com/bug970691_timestamp
Expands on basic patch to extract timestamp, print the timestamp in the rendered frame, and also print the delta between the received timestamp and current time. Can be demonstrated with WebRTC Test Landing page.
Needed:
1) ability to either determine fake video is inbound or have timestamp receive parsing configurable.
2) add a mechanism to determine initial frame size in anticipation of a future upgrade from the current manner in which the initial frame size is hardcoded to 640 x 480.
Preliminary review version. Running nightly with this patch will display a timestamp and latency number using the test session on the WebRTC Test page. Missing a mechanisim to turn on or off the timestamp extraction and display code in VideoConduit. I also need to understand how to expose this in stats so that tests (eg. Talos) can use the latency data.
Attachment #8395292 - Attachment is obsolete: true
Comment on attachment 8405041 [details] [diff] [review]
Part 1: Encode into and extract timestamps from fake video

This is a working version. Running nightly with this patch will display a timestamp and latency number using the test session on the WebRTC Test page. Missing a mechanisim to turn on or off the timestamp extraction and display code in VideoConduit. I also need to understand how to expose this in stats so that tests (eg. Talos) can use the latency data.
To complete, I need more details on how you want this stored in stats or telemetry.
Attachment #8405041 - Flags: review?(rjesup)
Comment on attachment 8405041 [details] [diff] [review]
Part 1: Encode into and extract timestamps from fake video

Review of attachment 8405041 [details] [diff] [review]:
-----------------------------------------------------------------

We can in the future replace this with a a 2d barcode probably if the API is reasonable. I'd suggest it encode (and decode) by taking input position as advisory and adjusting if needed, instead of failing.

The size issue is serious I think; it needs to be bigger, and it needs higher data integrity if we're using this for tests.

::: content/media/webrtc/MediaEngineDefault.cpp
@@ +208,5 @@
>    return NS_OK;
>  #endif
>  }
>  
> +

remove spurious blank line

@@ +243,5 @@
>    layers::PlanarYCbCrData data;
>    AllocateSolidColorFrame(data, mOpts.mWidth, mOpts.mHeight, 0x80, mCb, mCr);
> +
> +  YuvTimeStamp meta;
> +  memset(&meta, 0, sizeof(meta));

remove memset.  If it needs init, make it a class with an init, but I think just remove memset here.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +1047,5 @@
>    CSFLogDebug(logTag,  "%s Buffer Size %d", __FUNCTION__, buffer_size);
>  
> +  if (mReceivingWidth && mReceivingHeight) {
> +    YuvTimeStamp meta;
> +    memset(&meta, 0, sizeof(meta));

remove memset unless Decode can return TRUE without setting .magic

@@ +1052,5 @@
> +    bool rv = YuvStamper::Decode(mReceivingWidth, mReceivingHeight, mReceivingWidth,
> +                                 buffer,
> +                                 reinterpret_cast<uint8_t*>(&meta),
> +                                 sizeof(meta), 0, 0);
> +    if (rv & (meta.magic == YuvTimeStampMagic)) {

&&!

::: media/webrtc/signaling/src/media-conduit/YuvStamper.cpp
@@ +15,5 @@
> + 1, 0, 0, 0, 0, 1,
> + 1, 0, 0, 0, 0, 1,
> + 0, 1, 0, 0, 1, 0,
> + 0, 0, 1, 1, 0, 0
> +};

unsigned char's?  I realize I'm an old fart, but let's use an 1x8 array of bitmasks in uint8_t's
And the k-constants should appear int the array size here to avoid fubars

@@ +140,5 @@
> +  if (y > height_)
> +    return false;
> +
> +  uint8_t *ptr = &data[y * stride_ + x];
> +  *ptr = (*ptr > 96) ? 0 : 0xff;

evil constant.  And why the roundabout use of a pointer instead of direct array reference?

Also, 0x00 and 0xff are not valid Y values.  Min is 0x10, max is IIRC 240

@@ +150,5 @@
> +  if (x > width_)
> +    return false;
> +
> +  if (y > height_)
> +    return false;

ditto

@@ +158,5 @@
> +
> +  return true;
> +}
> +
> +bool YuvStamper::ReadValue(uint8_t *data, uint32_t x, uint32_t y, uint8_t* value) {

ReadValue and WriteValue are pretty much unnecessary.  Bounds-checking could be done once before encoding/decoding instead of on every pixel.  
At best they should be inline functions.

@@ +211,5 @@
> +}
> +
> +// Encode a binary value as a big-endian binary value starting at x, y,
> +// with 1 being a 2x2 block of 0x80 pixels and 0 being a 2x2 block of
> +// 0 pixels.

Needs to be bigger - we may adjust resolution on the fly, and the codec may blur it

@@ +213,5 @@
> +// Encode a binary value as a big-endian binary value starting at x, y,
> +// with 1 being a 2x2 block of 0x80 pixels and 0 being a 2x2 block of
> +// 0 pixels.
> +bool YuvStamper::Encode(uint32_t width, uint32_t height, uint32_t stride,
> +                        uint8_t* data, const uint8_t* value, size_t len,

use mozilla naming.
data should be something like 'aLuma' or "aYData"

@@ +223,5 @@
> +    for (uint32_t xx = 0; xx < kBitSize; ++xx) {
> +      for (uint32_t yy = 0; yy < kBitSize; ++yy) {
> +        if (!stamper.WriteValue(data, x + (i * kBitSize) + xx, y +yy,
> +                                is_set ? 0x80 : 0))
> +          return false;

Might be better to have stuff that runs past the right edge go down a line, in case the video we're encoding into changes to a much lower resolution.

@@ +239,5 @@
> +                        uint8_t* data, uint8_t* value, size_t len,
> +                        uint32_t x, uint32_t y) {
> +  YuvStamper stamper(width, height, stride);
> +
> +  memset(value, 0, len);

This code guarantees it sets value, or it returns failure.  Since we can return early here, we can memset, but it shouldn't be needed. -- or memset only on failure if we need to be paranoid

::: media/webrtc/signaling/src/media-conduit/YuvStamper.h
@@ +16,5 @@
> +                                         uint32_t stride,
> +                                         uint8_t *data,
> +                                         const T& value,
> +                                         uint32_t x=0,
> +                                         uint32_t y=0) {

use { on left for functions

@@ +44,5 @@
> +  const static uint32_t kBitThreshold = 60;
> +
> +  uint32_t width_;
> +  uint32_t height_;
> +  uint32_t stride_;

Mozilla naming standards please, not Google, for new code. There's no upstream here

@@ +48,5 @@
> +  uint32_t stride_;
> +};
> +
> +
> +static const uint32_t YuvTimeStampMagic = 0x564d444d;

Should be a CRC value or even simple checksum

@@ +51,5 @@
> +
> +static const uint32_t YuvTimeStampMagic = 0x564d444d;
> +struct YuvTimeStamp {
> +  uint32_t magic;
> +  uint64_t timestamp;

mFoo
Attachment #8405041 - Flags: review?(rjesup) → review-
Removed digit printing code as extraneous to timestamp encode/decode latency test function. Increased size of encoded bit. Added ability to wrap encoding line within the frame. Added latency average stat collection to VideoConduit.
Attachment #8405041 - Attachment is obsolete: true
Comment on attachment 8408258 [details] [diff] [review]
Part 1: Encode into and extract timestamps from fake video

Removed digit printing code as extraneous to timestamp encode/decode latency test function. Increased size of encoded bit. Added ability to wrap encoding line within the frame. Added latency average stat collection: VideoConduit::mozVideoLatencyAvg().

Enable timestamp parsing and latency data collection using "media.navigator.video.test_latency" pref. Should this be tied to something like build_for_test instead?
Attachment #8408258 - Flags: review?(rjesup)
Paul,

It would be really convenient to have that digit printing code in-tree. I use it a lot
and having it sit on some extra patchset is not really that convenient. I have another
set of patches that actually use it to create a HUD of the current system statistics,
but that lives in other files. Is there any really compelling reason to remove them?
(In reply to Eric Rescorla (:ekr) from comment #21)
> Paul,
> 
> It would be really convenient to have that digit printing code in-tree. I
> use it a lot
> and having it sit on some extra patchset is not really that convenient. I
> have another
> set of patches that actually use it to create a HUD of the current system
> statistics,
> but that lives in other files. Is there any really compelling reason to
> remove them?

The reason that I removed them was to get a review of the functionality that I think (I could be wrong here) that Jesup wanted for this bug: encoded transport of the timestamp to be used to test video latency. I treated your stamper as a starting point. The digit printing functions can coexist with the encode/decode function; I would need to spend a little time incorporating Jesup's review comments.
(In reply to Paul Kerr [:pkerr] from comment #22)
> (In reply to Eric Rescorla (:ekr) from comment #21)
> > Paul,
> > 
> > It would be really convenient to have that digit printing code in-tree. I
> > use it a lot
> > and having it sit on some extra patchset is not really that convenient. I
> > have another
> > set of patches that actually use it to create a HUD of the current system
> > statistics,
> > but that lives in other files. Is there any really compelling reason to
> > remove them?
> 
> The reason that I removed them was to get a review of the functionality that
> I think (I could be wrong here) that Jesup wanted for this bug: encoded
> transport of the timestamp to be used to test video latency. I treated your
> stamper as a starting point. The digit printing functions can coexist with
> the encode/decode function; I would need to spend a little time
> incorporating Jesup's review comments.

Would you do that, please. Seems like it would be good to land this and minimize
the bit rot.
Comment on attachment 8408258 [details] [diff] [review]
Part 1: Encode into and extract timestamps from fake video

Review of attachment 8408258 [details] [diff] [review]:
-----------------------------------------------------------------

r- mostly for the AdvanceCursor bit.  The rest are nits, style and "would be better" stuff that could be addressed easily.

::: content/media/webrtc/MediaEngineDefault.cpp
@@ +208,5 @@
>    return NS_OK;
>  #endif
>  }
>  
> +

extraneous blank line

::: media/webrtc/signaling/src/media-conduit/MediaConduitInterface.h
@@ +267,5 @@
>      bool UsingNackBasic() const {
>        return mUsingNackBasic;
>      }
>  
> +    void VideoTestLatency(bool enable) {mVideoTestLatency = true;}

This looks to be an obvious bug.  Also spaces inside the braces to match style.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.h
@@ +288,5 @@
> +  static const unsigned int sAlphaNum = 7;
> +  static const unsigned int sAlphaDen = 8;
> +  static const unsigned int sRoundingPadding = 1024;
> +  void MozVideoLatencyUpdate(uint64_t new_sample);
> +  uint64_t MozVideoLatencyAvg();

generally put functions above all the data defs

@@ +293,3 @@
>  
>    mozilla::RefPtr<WebrtcAudioConduit> mSyncedTo;
> +

spurious blank line

::: media/webrtc/signaling/src/media-conduit/YuvStamper.cpp
@@ +35,5 @@
> +    {
> +      return false;
> +    }
> +
> +    uint32_t crc = htonl(Crc32(pMsg, msg_len));

nit: Just call r_crc32() directly here; no need to wrap it in a method.

@@ +49,5 @@
> +    stamper.Write8(*pCrc++);
> +    stamper.Write8(*pCrc++);
> +
> +    return true;
> +  } 

trailing space

@@ +88,5 @@
> +
> +  void YuvStamper::Write8(uint8_t value)
> +  {
> +    // Encode MSB to LSB.
> +    WriteBit(value & 0x80);

uint8_t mask = 0x80;
while (mask) {
 WriteBit(!!(value & mask));
 mask >>= 1;
}

Seems cleaner to me (and matches Read8), but either will do.  Note if you don't do that, you still need the !!(...) - bool, not uint8_t to WriteBit
Even cleaner perhaps: WriteBit into that loop.  Again though not mandatory

@@ +111,5 @@
> +
> +    return AdvanceCursor(1);
> +  }
> +
> +  uint32_t YuvStamper::AdvanceCursor(uint32_t num_bits)

The reality is you can't use num_bits > 1, since this only checks for enough space to advance the cursor.  In fact, even that fails since it checks for enough space to move the cursor, not that there is even enough space for one more bit on the row.  In and extreme case, this could go past the end of the buffer and overwrite other memory.

I suggest either making it one bit always (and check for space for it), or tell it how many bits to reserve space for as well.

@@ +113,5 @@
> +  }
> +
> +  uint32_t YuvStamper::AdvanceCursor(uint32_t num_bits)
> +  {
> +    if (!num_bits) return 0;

a) for numerics, use == 0
b) brace and don't use if () { foo } on one line

@@ +117,5 @@
> +    if (!num_bits) return 0;
> +    // Space across an infinite row.
> +    uint32_t x_needed = num_bits * sBitSize;
> +
> +    for (;;) {

while (true) {

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +1688,5 @@
>  
> +#ifdef MOZILLA_INTERNAL_API
> +  if (mozilla::Preferences::GetBool("media.navigator.video.test_latency", false)) {
> +    conduit->VideoTestLatency(true);
> +  }

Much like the prefs in AudioConduit, this could move to ConfigureSend/RecvMediaCodec(), or even to the VideoConduit constructor, and avoid adding an external method.  Prefs do need to be accessed on MainThread
Attachment #8408258 - Flags: review?(rjesup) → review-
Updated based on review feedback.
Attachment #8408258 - Attachment is obsolete: true
Attachment #8408503 - Flags: review?(rjesup)
Comment on attachment 8408503 [details] [diff] [review]
Part 1: Encode into and extract timestamps from fake video

Review of attachment 8408503 [details] [diff] [review]:
-----------------------------------------------------------------

r- for the Advance issue
Also, we need ekr's digit functions.  I'm ok if that's a follow-on patch 2 (and thus can be reviewed/land separately), but he needs it.

::: media/webrtc/signaling/src/media-conduit/MediaConduitInterface.h
@@ +187,5 @@
>    };
>  
> +  VideoSessionConduit() :
> +      mFrameRequestMethod(FrameRequestNone),
> +      mUsingNackBasic(false) {}

no changes here, don't touch file

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +1062,5 @@
>                                   int64_t render_time,
>                                   void *handle)
>  {
>    CSFLogDebug(logTag,  "%s Buffer Size %d", __FUNCTION__, buffer_size);
> +  NS_IsMainThread();

NS_IsMainThread() alone does nothing... remove

::: media/webrtc/signaling/src/media-conduit/YuvStamper.cpp
@@ +9,5 @@
> +#endif
> +
> +#include "YuvStamper.h"
> +
> +typedef uint32_t UINT4;

unused typedef

@@ +55,5 @@
> +    stamper.Write8(*pCrc++);
> +    stamper.Write8(*pCrc++);
> +
> +    return true;
> +  } 

trailing ws

@@ +120,5 @@
> +  }
> +
> +  bool YuvStamper::AdvanceCursor()
> +  {
> +    if ((mCursor.x += sBitSize) >= mWidth) {

Still doesn't guarantee there's space for a bit starting at this location.  You need to make sure that in the new position, there are sBitSize pixels still available *after* the cursor position.

Also, too error-prone/hard-to-read to have an "if ((foo += bar) >= qwe)"  Separate them:
foo += bar;
if (foo >= qwe) {

The only advantage of the usage given is that it uses one less line vertically on the screen.

@@ +122,5 @@
> +  bool YuvStamper::AdvanceCursor()
> +  {
> +    if ((mCursor.x += sBitSize) >= mWidth) {
> +      // move to the start of the next row if possible.
> +      if ((mCursor.y += sBitSize) < mHeight) {

ditto to both

::: media/webrtc/signaling/src/media-conduit/YuvStamper.h
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +namespace mozilla {
> +
> +class 

delete trailing whitespace, put class on same line as YUVStamper
Attachment #8408503 - Flags: review?(rjesup) → review-
> 
> @@ +120,5 @@
> > +  }
> > +
> > +  bool YuvStamper::AdvanceCursor()
> > +  {
> > +    if ((mCursor.x += sBitSize) >= mWidth) {
> 
> Still doesn't guarantee there's space for a bit starting at this location. 
> You need to make sure that in the new position, there are sBitSize pixels
> still available *after* the cursor position.
> 
The mWidth and mLength are adjusted when the Stamper instance is created to an integral number of sBitSizes in the constructor. If mCursor.x + sBitSize is less than mWidth there should be at least on sBitSize number of luma bits left in the line. There should be no remainder less than sBitSize.
I will make this clearer.
(In reply to Eric Rescorla (:ekr) from comment #23)
> (In reply to Paul Kerr [:pkerr] from comment #22)
> > (In reply to Eric Rescorla (:ekr) from comment #21)
> > > Paul,
> > > 
> > > It would be really convenient to have that digit printing code in-tree. I
> > > use it a lot
> > > and having it sit on some extra patchset is not really that convenient. I
> > > have another
> > > set of patches that actually use it to create a HUD of the current system
> > > statistics,
> > > but that lives in other files. Is there any really compelling reason to
> > > remove them?
> > 
> > The reason that I removed them was to get a review of the functionality that
> > I think (I could be wrong here) that Jesup wanted for this bug: encoded
> > transport of the timestamp to be used to test video latency. I treated your
> > stamper as a starting point. The digit printing functions can coexist with
> > the encode/decode function; I would need to spend a little time
> > incorporating Jesup's review comments.
> 
> Would you do that, please. Seems like it would be good to land this and
> minimize
> the bit rot.

A second patch is in the works. Need to get a + on the first before dropping it.
(In reply to Paul Kerr [:pkerr] from comment #27)
> > 
> > @@ +120,5 @@
> > > +  }
> > > +
> > > +  bool YuvStamper::AdvanceCursor()
> > > +  {
> > > +    if ((mCursor.x += sBitSize) >= mWidth) {
> > 
> > Still doesn't guarantee there's space for a bit starting at this location. 
> > You need to make sure that in the new position, there are sBitSize pixels
> > still available *after* the cursor position.
> > 
> The mWidth and mLength are adjusted when the Stamper instance is created to
> an integral number of sBitSizes in the constructor. If mCursor.x + sBitSize
> is less than mWidth there should be at least on sBitSize number of luma bits
> left in the line. There should be no remainder less than sBitSize.
> I will make this clearer.

Looking at this again, I think it would leave less things that could go wrong in future code modifications to move all the logic for the handling cursor advancement and space checking into one location: AdvanceCursor(), than have AdvanceCursor() assume that the correct thing has happened in the constructor.
Incorporated review feedback. Added second patch to bring forward digit printing fuctionality from the source YuvStamper.
Attachment #8408503 - Flags: review- → review?(rjesup)
Attachment #8409392 - Flags: review?(rjesup)
Comment on attachment 8408503 [details] [diff] [review]
Part 1: Encode into and extract timestamps from fake video

Review of attachment 8408503 [details] [diff] [review]:
-----------------------------------------------------------------

Not sure if you meant to put this up for re-review after your comment.  

r+ with nits and switching >= mWidth/mHeight in AdvanceCursor() to > mWidth/mHeight, and adding a comment block to explain - or fix it to all happen in AdvanceCursor as you indicated, which is preferable, and have a quick re-review.

::: media/webrtc/signaling/src/media-conduit/YuvStamper.cpp
@@ +23,5 @@
> +			 uint32_t stride,
> +			 uint32_t x,
> +			 uint32_t y):
> +    pYData(pYData), mStride(stride),
> +    mWidth(width - width % sBitSize),

This does not actually reserve the space for one bit; it just makes sure width is a multiple of sBitSize.

@@ +120,5 @@
> +  }
> +
> +  bool YuvStamper::AdvanceCursor()
> +  {
> +    if ((mCursor.x += sBitSize) >= mWidth) {

Given that the constructor sets mWidth to be a multiple of sBitSize (and the initial cursor x and y), then after mCursor.x += sBitSize, there will be an integral number of sBitSize units left.  However, that value can be 0, so this still fails since the test is >= mWidth.
Comment on attachment 8409392 [details] [diff] [review]
Part2: Restore digit stamping function to utility

Review of attachment 8409392 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with nits

::: media/webrtc/signaling/src/media-conduit/YuvStamper.cpp
@@ +32,5 @@
> +  1, 0, 0, 0, 0, 1,
> +  0, 1, 0, 0, 1, 0,
> +  0, 0, 1, 1, 0, 0
> +*/
> +static unsigned char DIGIT_0 [] =

s/unsigned_char/uint8_t/
Also, [sDigitHeight] to avoid foobars

@@ +33,5 @@
> +  0, 1, 0, 0, 1, 0,
> +  0, 0, 1, 1, 0, 0
> +*/
> +static unsigned char DIGIT_0 [] =
> +  { 0,

Why leading 0 on all of them?  Is this some form of inter-line separation?  If so, I'd rather that be explicit in the loop.  However, it's not critical, but at least comment on what it is

@@ +42,5 @@
> +    ON_5 | ON_0,
> +    ON_4 | ON_1,
> +    ON_3 | ON_2
> +  };
> +    

trailing ws

@@ +251,1 @@
>      mCursor(x, y) {}

MOZ_ASSERT(symbol_width <= 8)
Since the code breaks otherwise

@@ +414,4 @@
>      return AdvanceCursor();
>    }
>  
> +  bool YuvStamper::Write(uint32_t value)

Rename WriteDigits() to avoid confusion

::: media/webrtc/signaling/src/media-conduit/YuvStamper.h
@@ +3,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  namespace mozilla {
>  
>  class 

trailing ws

@@ +51,2 @@
>    const static uint32_t sBitSize = 4;
> +  const static uint32_t sBitThreshold = 60;

why did this change?  Why is this the right value?

@@ +53,3 @@
>    const static uint8_t sYOn = 0x80;
>    const static uint8_t sYOff = 0;
> +  const static uint8_t sLumaThreshold = 96;

Why is this the right value?  Just want to see justification
Attachment #8409392 - Flags: review?(rjesup) → review+
Comment on attachment 8408503 [details] [diff] [review]
Part 1: Encode into and extract timestamps from fake video

Forgot to set
Attachment #8408503 - Flags: review?(rjesup) → review+
Review feedback
Attachment #8408503 - Attachment is obsolete: true
Review feedback. Digit spacing controlled by class constants.
Attachment #8409392 - Attachment is obsolete: true
>@@ +53,3 @@
>>    const static uint8_t sYOn = 0x80;
>>    const static uint8_t sYOff = 0;
>> +  const static uint8_t sLumaThreshold = 96;
>
>Why is this the right value?  Just want to see justification

ekr, can you provide background on the choice of 96 as the threshold level in the original YuvStamper::WritePixel and the selection of 60 as the YuvStamper::Decode test threshold.
Flags: needinfo?(ekr)
I got them from ben brittain who I believe got them from derf.
Flags: needinfo?(ekr) → needinfo?(tterribe)
(In reply to Eric Rescorla (:ekr) from comment #37)
> I got them from ben brittain who I believe got them from derf.

If I gave bbrittain these, I don't remember doing so. My best guess was that when he tried it, it looked okay and worked in testing.

BTW, sLumaMax should be 235 instead of 240 for standard REC 601 Y'CbCr.
Flags: needinfo?(tterribe)
Linux build fix
Attachment #8409702 - Attachment is obsolete: true
(In reply to Timothy B. Terriberry (:derf) from comment #38)
> (In reply to Eric Rescorla (:ekr) from comment #37)
> > I got them from ben brittain who I believe got them from derf.
> 
> If I gave bbrittain these, I don't remember doing so. My best guess was that
> when he tried it, it looked okay and worked in testing.
> 
> BTW, sLumaMax should be 235 instead of 240 for standard REC 601 Y'CbCr.

I can confirm that the result does work well against the fake video background and live video.
So.... these patches should be up-to-date and marked r+ and have descriptions that are ready for checking (including r= lines), then mark the bug for checkin-needed (in keywords).  If there was a change needing re-review, please put them up.
description update and application against latest version of inbound
Attachment #8409701 - Attachment is obsolete: true
description update and application against latest version of inbound
Attachment #8409790 - Attachment is obsolete: true
Keywords: checkin-needed
generate missing typedefs for win32 gecko build
Attachment #8411152 - Attachment is obsolete: true
generate missing typedefs for win32 gecko build
Attachment #8411155 - Attachment is obsolete: true
(In reply to Wes Kocher (:KWierso) from comment #45)
> Backed out in
> http://hg.mozilla.org/integration/mozilla-inbound/rev/d8014e46546e for build
> bustage:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=38359030&tree=Mozilla-
> Inbound

Added nptypes.h to handle undefined integer types in the win32 gecko build. Ran a successful b2g desktop (win32) build.
Flags: needinfo?(pkerr)
strlen import for B2G
Attachment #8412316 - Attachment is obsolete: true
Keywords: checkin-needed
Depends on: 1001708
Depends on: 1004194
Whiteboard: [s=fx32]
Whiteboard: [s=fx32] → [p=3, 1.5:p1, ft:webrtc]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: