1015856 - [MediaEncoder::GTest] Testcase for bug 970774, WebM aspect ratio.

Status: RESOLVED FIXED

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

Comment 1

[Benjamin Chen [:bechen]]

Attachment: bug-1015856.patch

Comment 2

[Benjamin Chen [:bechen]]

Attachment: bug-1015856.patch

Rebase.
try server: https://tbpl.mozilla.org/?tree=Try&rev=f87924abc2c2

Comment 3

[Ralph Giles (:rillian)]

Comment on attachment 8434660 [details] [diff] [review]
bug-1015856.patch

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

r- for overflow bugs. This test won't trigger them, but I don't want bad code in the tree where someone might copy it.

Please address the comments and ask for review on the updated patch.

::: content/media/gtest/TestWebMWriter.cpp
@@ +228,5 @@
>  }
> +
> +struct WebMioData {
> +  nsTArray<uint8_t> data;
> +  int64_t offset;

Better to use size_t here, since it's an array index.

@@ +231,5 @@
> +  nsTArray<uint8_t> data;
> +  int64_t offset;
> +};
> +
> +static int webm_read(void *aBuffer, size_t aLength, void *aUserData)

Please move pointer '*' to the type name to match Mozilla C++ style:

static int webm_read(void* aBuffer, size_t aLength, void* aUserData)

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Declarations

@@ +234,5 @@
> +
> +static int webm_read(void *aBuffer, size_t aLength, void *aUserData)
> +{
> +  NS_ASSERTION(aUserData, "aUserData must point to a valid WebMioData");
> +  WebMioData* ioData = reinterpret_cast<WebMioData*>(aUserData);

static_cast<> here and below instead, please.

It doesn't really make a difference for re-typing a void*, but dynamic can have side-effects in other contexts and static is what the other other tests in this directory are using.

@@ +244,5 @@
> +  size_t readLength = aLength;
> +  if (ioData->offset + readLength > ioData->data.Length()){
> +    readLength = ioData->data.Length() - ioData->offset;
> +  }
> +  if (readLength > 0) {

readLength is unsigned, so this doesn't clamp the copied data to the end of the array the way you want. Compare the unsigned lengths directly and return error if the requested data goes past the end.

@@ +259,5 @@
> +  WebMioData* ioData = reinterpret_cast<WebMioData*>(aUserData);
> +
> +  int64_t oldOffset = ioData->offset;
> +  switch (aWhence) {
> +    case PR_SEEK_END:

Please use NESTEGG_SEEK_* here instead. One less include, and the define the library will actually use with this callback.

@@ +273,5 @@
> +      NS_ERROR("Unknown whence");
> +      return -1;
> +  }
> +
> +  if (ioData->offset < 0 || ioData->offset > ioData->data.Length()) {

Again, this can overflow. Please use CheckedInt or direct comparisons before arithmetic.

@@ +284,5 @@
> +static int64_t webm_tell(void *aUserData)
> +{
> +  NS_ASSERTION(aUserData, "aUserData must point to a valid WebMioData");
> +  WebMioData* ioData = reinterpret_cast<WebMioData*>(aUserData);
> +  return ioData->offset;

Compare against INT64_MAX and return and error if this overflows the return type.

@@ +313,5 @@
> +
> +  // Get the metadata and the first cluster.
> +  nsTArray<nsTArray<uint8_t> > encodedBuf;
> +  writer.GetContainerData(&encodedBuf, 0);
> +  // Flat the encodedBuf.

// Flatten the encodedBuf.

@@ +326,5 @@
> +  nestegg_io io;
> +  io.read = webm_read;
> +  io.seek = webm_seek;
> +  io.tell = webm_tell;
> +  io.userdata = (void *)&ioData;

(void*)&ioData or static_cast<void*>(&ioData)

@@ +327,5 @@
> +  io.read = webm_read;
> +  io.seek = webm_seek;
> +  io.tell = webm_tell;
> +  io.userdata = (void *)&ioData;
> +  int r = nestegg_init(&context, io, nullptr, -1);

rv is more common mozilla style for checked return values.

@@ +328,5 @@
> +  io.seek = webm_seek;
> +  io.tell = webm_tell;
> +  io.userdata = (void *)&ioData;
> +  int r = nestegg_init(&context, io, nullptr, -1);
> +  EXPECT_TRUE((r != -1));

Better to check for the expected value here. EXPECT_EQ(rv, 0)

@@ +331,5 @@
> +  int r = nestegg_init(&context, io, nullptr, -1);
> +  EXPECT_TRUE((r != -1));
> +  unsigned int ntracks = 0;
> +  r = nestegg_track_count(context, &ntracks);
> +  EXPECT_TRUE((r != -1) && (ntracks == 2));

Separate tests will report better on failure:

EXPECT_EQ(rv, 0);
EXPECT_EQ(ntracks, 2);

@@ +332,5 @@
> +  EXPECT_TRUE((r != -1));
> +  unsigned int ntracks = 0;
> +  r = nestegg_track_count(context, &ntracks);
> +  EXPECT_TRUE((r != -1) && (ntracks == 2));
> +  for (uint32_t track = 0; track < ntracks; ++track) {

unsigned int track

@@ +340,5 @@
> +    if (type == NESTEGG_TRACK_VIDEO) {
> +      nestegg_video_params params;
> +      r = nestegg_track_video_params(context, track, &params);
> +      EXPECT_TRUE((r != -1));
> +      EXPECT_TRUE(width == params.width);

The try build fails here because of the signed vs unsigned comparison. I don't see a way to make the types align between the WebMWriter an nestegg APIs. static_cast should be safe here since you're just checking equality.

Again, I prefer EXPECT_EQ(width, params.width);

Comment 4

[Benjamin Chen [:bechen]]

Attachment: bug-1015856.patch

Thanks for reviewing in detail.

> @@ +284,5 @@
> > +static int64_t webm_tell(void *aUserData)
> > +{
> > +  NS_ASSERTION(aUserData, "aUserData must point to a valid WebMioData");
> > +  WebMioData* ioData = reinterpret_cast<WebMioData*>(aUserData);
> > +  return ioData->offset;
> 
> Compare against INT64_MAX and return and error if this overflows the return
> type.

Since I change the |offset| to size_t, do I still need the check?

Comment 5

[Ralph Giles (:rillian)]

Comment on attachment 8441233 [details] [diff] [review]
bug-1015856.patch

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

Thanks for making the requested changes. This looks cleaner. I'd still like you check for overflow in seek and tell methods though.

You can add specific checks, or try |CheckedInt<size_t> newOffset| and 

  if (!newOffset.isValid()) {
    NS_ERROR("invalid offset");
    return -1;
  }
  ioData->offset = newOffset.value();

at the end.

::: content/media/gtest/TestWebMWriter.cpp
@@ +6,5 @@
>  #include "gtest/gtest.h"
>  #include "VorbisTrackEncoder.h"
>  #include "VP8TrackEncoder.h"
>  #include "WebMWriter.h"
> +#include "nestegg/nestegg.h"

Please put this after gtest.h for alphabetical order.

@@ +239,5 @@
> +  // Check eos
> +  if (ioData->offset >= ioData->data.Length()) {
> +    return 0;
> +  }
> +  if (ioData->offset + aLength > ioData->data.Length()) {

This is a better check, but can still overflow and pass incorrectly on 32 bit systems.

@@ +254,5 @@
> +  WebMioData* ioData = static_cast<WebMioData*>(aUserData);
> +
> +  size_t oldOffset = ioData->offset;
> +  switch (aWhence) {
> +    case NESTEGG_SEEK_END:

if (aOffset > 0 || -aOffset > ioData->data.Length()) {
  NS_ERROR("invalid offset");
  return -1;
}

@@ +258,5 @@
> +    case NESTEGG_SEEK_END:
> +      ioData->offset = ioData->data.Length() + aOffset;
> +      break;
> +    case NESTEGG_SEEK_CUR:
> +      ioData->offset += aOffset;

This can overflow similarly on 32 bit systems.

@@ +279,5 @@
> +static int64_t webm_tell(void* aUserData)
> +{
> +  NS_ASSERTION(aUserData, "aUserData must point to a valid WebMioData");
> +  WebMioData* ioData = static_cast<WebMioData*>(aUserData);
> +  return ioData->offset;

Yes, you still need to check for overflow here. |offset| is a size_t which has a larger maximum value than the int64_t return type on 64-bit systems. If you return a value > INT64_t it will be aliased to a negative offset.

In practice nestegg seems to just check for negative values, but the documented return code for error is -1, so you should return that.

Comment 6

[Benjamin Chen [:bechen]]

Attachment: bug-1015856.patch

Address the overflow bug by CheckedInt.
Is that ok to use Abs() to check the input argument in my patch?

try server:
https://tbpl.mozilla.org/?tree=Try&rev=4b6ddd52e14e

Comment 7

[Ralph Giles (:rillian)]

Comment on attachment 8446331 [details] [diff] [review]
bug-1015856.patch

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

Thanks. r=me with last nit addressed.

cpearce reminded me having offset be size_t isn't ideal as model code since that's a 32-bit type: too small for real media files. OTOH, keeping more than 4 GB in ram isn't a good example either. I think it's ok for this use.

::: content/media/gtest/TestWebMWriter.cpp
@@ +247,5 @@
> +  // Check eos.
> +  if (ioData->offset >= ioData->data.Length()) {
> +    return 0;
> +  }
> +  CheckedInt<size_t> newOffset = ioData->offset + aLength;

Unfortunately this evaluates the expression before converting to CheckedInt, so there's no overflow check here. At least one of the arguments to '+' must already be a CheckedInt for the operator method to be invoked.

You could make WebMioData::offset a CheckedInt<> or just do:

CheckedInt<size_t> newOffset = ioData->offset;
newOffset += aLength;
if (!newOffset.isValid || ...)

@@ +262,5 @@
> +{
> +  NS_ASSERTION(aUserData, "aUserData must point to a valid WebMioData");
> +  WebMioData* ioData = static_cast<WebMioData*>(aUserData);
> +
> +  if (Abs(aOffset) > ioData->data.Length()) {

This should work fine.

Comment 8

[Benjamin Chen [:bechen]]

Attachment: bug-1015856.patch

Address the overflow codes by change the WebMioData::offset to CheckedInt<>.
r=rillian
try server:https://tbpl.mozilla.org/?tree=Try&rev=0081fe9dd3e1

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/93b9e0b5a34b

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/93b9e0b5a34b