Add unit tests for avfoundation format mapper functions and fix wrong implementation.

webrtc/sdk/objc/Framework/UnitTests/avformatmappertests.mm

+/*
+ *  Copyright 2016 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#import <Foundation/Foundation.h>
+#import <OCMock/OCMock.h>
+#include "avfoundationformatmapper.h"
+#include "webrtc/base/gunit.h"
+
+static NSString * const kMediaSubtypeKey = @"subtype";
+static NSString * const kMinFrameRateKey = @"min_framerate";
+static NSString * const kMaxFrameRateKey = @"max_framerate";
+
+// Width and height don't play any role so lets use predefined values throughout
+// the tests.
+static const int kFormatWidth = 789;
+static const int kFormatHeight = 987;
+
+// Hardcoded framrate to be used throughout the tests.
+static const int kFramerate = 30;
+
+
+NSArray<AVCaptureDeviceFormat*>* GetEligibleDeviceFormats(
+    const AVCaptureDevice* device,
+    int supportedFps);
+AVCaptureDeviceFormat* GetDeviceFormatForVideoFormat(
+    const AVCaptureDevice* device,
+    const cricket::VideoFormat& videoFormat);
+
+
+// Mock class for AVCaptureDeviceFormat.
+// Custom implementation needed because OCMock cannot handle the
+// CMVideoDescriptionRef mocking.
+@interface AVCaptureDeviceFormatMock : NSObject {
+  CMVideoFormatDescriptionRef _format;
+  OCMockObject *_rangeMock;
+}
+
+// The dictionaries are in the following format.
+// @{@"subtype":@(kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange),
+//   @"min_framerate":@0,
+//   @"max_framerate":@30}
+
+- (instancetype)initWithDictionary:(NSDictionary *)mockDataDictionary;
+@end
+
+@implementation AVCaptureDeviceFormatMock
+
+- (instancetype)initWithDictionary:(NSDictionary *)mockDataDictionary {
+  if (self = [super init]) {
+    NSNumber *subtype = [mockDataDictionary objectForKey:kMediaSubtypeKey];

[magjed_webrtc, 2016/11/28 12:21:23]

What's the benefit of sending these arguments in an NSDictionary compared to
using explicit arguments?

[daniela-webrtc, 2016/11/29 12:05:55]

Done.

+    NSNumber *minFramerate = [mockDataDictionary objectForKey:kMinFrameRateKey];
+    NSNumber *maxFramerate = [mockDataDictionary objectForKey:kMaxFrameRateKey];
+
+
+    CMVideoFormatDescriptionCreate(nil, subtype.intValue, kFormatWidth,
+                                   kFormatHeight, nil, &_format);
+    // We can use OCMock for the range.
+    _rangeMock = [OCMockObject mockForClass:[AVFrameRateRange class]];
+    [[[_rangeMock stub] andReturnValue:@(minFramerate.floatValue)] minFrameRate];
+    [[[_rangeMock stub] andReturnValue:@(maxFramerate.floatValue)] maxFrameRate];
+  }
+
+  return self;
+}
+
+- (void)dealloc {
+  if (_rangeMock != nil) {
+    [_rangeMock dealloc];
+    _rangeMock = nil;
+  }
+  if (_format != nil) {
+    CFRelease(_format);
+    _format = nil;
+  }
+  [super dealloc];
+}
+
+// Redefinition of AVCaptureDevice methods we want to mock.
+- (CMVideoFormatDescriptionRef)formatDescription {
+  return _format;
+}
+
+- (NSArray *)videoSupportedFrameRateRanges {
+  return @[_rangeMock];
+}
+
+@end
+
+class AVFormatMapperTest : public ::testing::Test {
+ protected:
+  void testEligibleWithValidFormats() {

[magjed_webrtc, 2016/11/28 12:21:23]

It looks like you can make the unittests more simple if you only add one format
in every test. You can have one test with multiple formats to exercise that
logic, but the other unittests should be as simple as possible.

[daniela-webrtc, 2016/11/29 12:05:55]

Done.

[magjed_webrtc, 2016/11/29 13:34:01]

It's not done. What I mean is that if we want to unittest that invalidFpsFormat
is rejected, we should only add that format and not a valid format as well. I.e.
I want the test to look like this:

AVCaptureDeviceFormatMock* mockFormat =
    [AVCaptureDeviceFormatMock invalidFpsFormat];
[[[mockDevice stub] andReturn:@[ mockFormat ]] formats];
std::set<cricket::VideoFormat> result =
    webrtc::GetSupportedVideoFormatsForDevice(mockDevice);
EXPECT_TRUE(result.empty());

Same for testSuportedCricketFormatsWithInvalidMediaSubtypeFormats.

We can have a separate test with multiple formats, e.g. testMultipleFormats
where you add more than one format to the mock device.

+    // given
+    id mockDevice = [OCMockObject mockForClass:[AVCaptureDevice class]];
+
+    // Valid media subtype, valid framerate
+    AVCaptureDeviceFormatMock* mockOne =
+        [[AVCaptureDeviceFormatMock alloc] initWithDictionary:@{
+          kMediaSubtypeKey : @(kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange),
+          kMinFrameRateKey : @(0),
+          kMaxFrameRateKey : @(60)
+        }];
+    // Valid media subtype, valid framerate
+    AVCaptureDeviceFormatMock* mockTwo =
+        [[AVCaptureDeviceFormatMock alloc] initWithDictionary:@{
+          kMediaSubtypeKey : @(kCVPixelFormatType_420YpCbCr8PlanarFullRange),
+          kMinFrameRateKey : @(0),
+          kMaxFrameRateKey : @(30)
+        }];
+    [[[mockDevice stub] andReturn:@[ mockOne, mockTwo ]] formats];
+
+    // when
+    NSArray* result = GetEligibleDeviceFormats(mockDevice, kFramerate);
+
+    // then
+    EXPECT_TRUE(result.count == 2);

[magjed_webrtc, 2016/11/28 12:21:23]

EXPECT_EQ(2, result.count); Same for other places.

[daniela-webrtc, 2016/11/29 12:05:55]

Done.

+  }
+
+  void testEligibleWithInvalidFramerateFormats() {
+    // given
+    id mockDevice = [OCMockObject mockForClass:[AVCaptureDevice class]];
+
+    // Valid media subtype, invalid framerate
+    AVCaptureDeviceFormatMock* mockOne =
+        [[AVCaptureDeviceFormatMock alloc] initWithDictionary:@{
+          kMediaSubtypeKey : @(kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange),
+          kMinFrameRateKey : @(0),
+          kMaxFrameRateKey : @(20)
+        }];
+
+    // Valid media subtype, valid framerate
+    AVCaptureDeviceFormatMock* mockTwo =
+        [[AVCaptureDeviceFormatMock alloc] initWithDictionary:@{
+          kMediaSubtypeKey : @(kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange),
+          kMinFrameRateKey : @(0),
+          kMaxFrameRateKey : @(30)
+        }];
+
+    [[[mockDevice stub] andReturn:@[ mockOne, mockTwo ]] formats];
+
+    // when
+    NSArray* result = GetEligibleDeviceFormats(mockDevice, kFramerate);
+
+    // then
+    EXPECT_TRUE(result.count == 1);
+    EXPECT_TRUE([result[0] isEqual:mockTwo]);
+  }
+
+  void testEligibleWithInvalidFormats() {
+    // given
+    id mockDevice = [OCMockObject mockForClass:[AVCaptureDevice class]];
+
+    // Invalid media subtype, valid framerate
+    AVCaptureDeviceFormatMock* mockOne =
+        [[AVCaptureDeviceFormatMock alloc] initWithDictionary:@{
+          kMediaSubtypeKey : @(kCVPixelFormatType_420YpCbCr8Planar),
+          kMinFrameRateKey : @(0),
+          kMaxFrameRateKey : @(30)
+        }];
+
+    // Valid media subtype, valid framerate
+    AVCaptureDeviceFormatMock* mockTwo =
+        [[AVCaptureDeviceFormatMock alloc] initWithDictionary:@{
+          kMediaSubtypeKey : @(kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange),
+          kMinFrameRateKey : @(0),
+          kMaxFrameRateKey : @(30)
+        }];
+
+    [[[mockDevice stub] andReturn:@[ mockOne, mockTwo ]] formats];
+
+    // when
+    NSArray* result = GetEligibleDeviceFormats(mockDevice, kFramerate);
+
+    // then
+    EXPECT_TRUE(result.count == 1);
+    EXPECT_TRUE([result[0] isEqual:mockTwo]);
+  }
+
+  void testSuportedCricketFormats() {
+    // given
+    id mockDevice = [OCMockObject mockForClass:[AVCaptureDevice class]];
+
+    // valid media subtype, valid framerate
+    AVCaptureDeviceFormatMock* mockOne =
+        [[AVCaptureDeviceFormatMock alloc] initWithDictionary:@{
+          kMediaSubtypeKey : @(kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange),
+          kMinFrameRateKey : @(0),
+          kMaxFrameRateKey : @(30)
+        }];
+
+    [[[mockDevice stub] andReturn:@[ mockOne ]] formats];
+
+    cricket::VideoFormat expectedFormat = cricket::VideoFormat(
+        kFormatWidth, kFormatHeight,
+        cricket::VideoFormat::FpsToInterval(kFramerate), cricket::FOURCC_NV12);
+
+    // when
+    std::set<cricket::VideoFormat> result =
+        webrtc::GetSupportedVideoFormatsForDevice(mockDevice);
+
+    // then
+    EXPECT_EQ(result.size(), 1);

[magjed_webrtc, 2016/11/28 12:21:23]

You need to flip the order of the arguments, i.e. EXPECT_EQ(1, result.size()).

[daniela-webrtc, 2016/11/29 12:05:55]

Done.

+
+    // make sure the set has the expected format
+    auto it = result.find(expectedFormat);
+    EXPECT_TRUE(it != result.end());

[magjed_webrtc, 2016/11/28 12:21:23]

Do this instead: EXPECT_EQ(1u, result.count(expectedFormat));

[daniela-webrtc, 2016/11/29 12:05:55]

Done.

[magjed_webrtc, 2016/11/29 13:34:01]

Actually, since you only have one element, it might be better check that the
only element is equal to the expected format:
EXPECT_EQ(expectedFormat, *result.begin());

+  }
+
+  void testAVFormatForCricketFormat() {
+    // given
+    id mockDevice = [OCMockObject mockForClass:[AVCaptureDevice class]];
+
+    // valid media subtype, valid framerate
+    AVCaptureDeviceFormatMock* mockOne =
+        [[AVCaptureDeviceFormatMock alloc] initWithDictionary:@{
+          kMediaSubtypeKey : @(kCVPixelFormatType_420YpCbCr8PlanarFullRange),
+          kMinFrameRateKey : @(0),
+          kMaxFrameRateKey : @(30)
+        }];
+
+    // valid media subtype, valid framerate.
+    // This media subtype should be the preffered one.
+    AVCaptureDeviceFormatMock* mockTwo =
+        [[AVCaptureDeviceFormatMock alloc] initWithDictionary:@{
+          kMediaSubtypeKey : @(kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange),
+          kMinFrameRateKey : @(0),
+          kMaxFrameRateKey : @(30)
+        }];
+
+    [[[mockDevice stub] andReturn:@[ mockOne, mockTwo ]] formats];
+
+    cricket::VideoFormat inputFormat = cricket::VideoFormat(
+        kFormatWidth, kFormatHeight,
+        cricket::VideoFormat::FpsToInterval(kFramerate), cricket::FOURCC_NV12);
+
+    // when
+    id resultFormat = GetDeviceFormatForVideoFormat(mockDevice, inputFormat);
+
+    // then
+    EXPECT_TRUE([resultFormat isEqual:mockTwo]);

[magjed_webrtc, 2016/11/28 12:21:23]

So EXPECT_EQ doesn't work here?

[daniela-webrtc, 2016/11/29 12:05:55]

Done.

+  }
+
+  void testSetFormatWhenDeviceCannotLock() {
+    // given
+    id mockDevice = [OCMockObject mockForClass:[AVCaptureDevice class]];
+    [[[mockDevice stub] andReturnValue:@(NO)]
+        lockForConfiguration:[OCMArg setTo:nil]];
+
+    [[[mockDevice stub] andReturn:@[]] formats];
+
+    // when
+    bool resultFormat = webrtc::SetFormatForCaptureDevice(
+        mockDevice, nil, cricket::VideoFormat());
+
+    // then
+    EXPECT_FALSE(resultFormat);
+  }
+
+  void testSetFormatWhenFormatIsIncompatible() {
+    // given
+    id mockDevice = [OCMockObject mockForClass:[AVCaptureDevice class]];
+    [[[mockDevice stub] andReturn:@[]] formats];
+    [[[mockDevice stub] andReturnValue:@(YES)]
+        lockForConfiguration:[OCMArg setTo:nil]];
+
+    NSException* exception =
+        [NSException exceptionWithName:@"Test exception"
+                                reason:@"Raised from unit tests"
+                              userInfo:nil];
+    [[[mockDevice stub] andThrow:exception] setActiveFormat:[OCMArg any]];
+    [[mockDevice expect] unlockForConfiguration];
+
+    // when
+    bool resultFormat = webrtc::SetFormatForCaptureDevice(
+        mockDevice, nil, cricket::VideoFormat());
+
+    // then
+    EXPECT_FALSE(resultFormat);
+    [mockDevice verify];
+  }
+};
+
+TEST_F(AVFormatMapperTest, EligibleFormatsPerDeviceWithValidFormats) {
+  testEligibleWithValidFormats();

[magjed_webrtc, 2016/11/28 12:21:23]

Inline these functions instead, unless you have some good ObjC reason to do like
this.

[magjed_webrtc, 2016/11/29 13:34:01]

I still want this done. You don't need the fixture class AVFormatMapperTest at
all since your tests are so simple. So remove the class AVFormatMapperTest,
rename TEST_F to TEST, and inline the test function. I.e.:

TEST(AVFormatMapperTest, EligibleFormatsPerDeviceWithValidFormats) {
  // given
  id mockDevice = [OCMockObject mockForClass:[AVCaptureDevice class]];
  ...
}

[daniela-webrtc, 2016/11/29 16:13:57]

Sorry, I missed this one :/ It's changed now

+}
+
+TEST_F(AVFormatMapperTest,
+       EligibleFormatsPerDeviceWithInvalidFramerateFormats) {
+  testEligibleWithInvalidFramerateFormats();
+}
+
+TEST_F(AVFormatMapperTest, EligibleFormatsPerDeviceWithInvalidFormats) {
+  testEligibleWithInvalidFormats();
+}
+
+TEST_F(AVFormatMapperTest, SuportedCricketFormats) {
+  testSuportedCricketFormats();
+}
+
+TEST_F(AVFormatMapperTest, AVFormatForCricketFormat) {
+  testAVFormatForCricketFormat();
+}
+
+TEST_F(AVFormatMapperTest, SetFormatWhenDeviceCannotLock) {
+  testSetFormatWhenDeviceCannotLock();
+}
+
+TEST_F(AVFormatMapperTest, SetFormatWhenFormatIsIncompatible) {
+  testSetFormatWhenFormatIsIncompatible();
+}