Reciever basics by DLochmelis33 · Pull Request #4 · DLochmelis33/colorshare

DLochmelis33 · 2021-03-20T20:38:36Z

No description provided.

krasko · 2021-03-21T03:41:55Z

app/src/main/java/ru/hse/colorshare/ReceiverCameraActivity.java

+                    Comparator<String> cameraIdByResolutionComparator = new Comparator<String>() {
+                        private long convertIdToResolution(String id) {
+                            Size pixelArraySize = cameraCharacteristics.get(CameraCharacteristics.SENSOR_INFO_PIXEL_ARRAY_SIZE);
+                            return (long) pixelArraySize.getHeight() * pixelArraySize.getWidth();


Как я понимаю, цель введения long - избавиться от переполнения. В таком случае надо явно расставить скобки (читатель может не помнить, что приведение типов имеет больший приоритет, чем умножение).

krasko · 2021-03-21T03:42:18Z

app/src/main/java/ru/hse/colorshare/ReceiverCameraActivity.java

+                            long result = convertIdToResolution(o1) - convertIdToResolution(o2);
+                            if (result > 0) {
+                                return 1;
+                            } else if (result < 0) {
+                                return -1;
+                            }


return Long.compare(convertIdToResolution(o1), convertIdToResolution(o2));

krasko · 2021-03-21T03:45:28Z

app/src/main/java/ru/hse/colorshare/ReceiverCameraActivity.java

+                if (cameraCharacteristics.get(CameraCharacteristics.LENS_FACING) == CameraCharacteristics.LENS_FACING_BACK
+                        && cameraCharacteristics.get(CameraCharacteristics.SENSOR_INFO_COLOR_FILTER_ARRANGEMENT) != CameraCharacteristics.SENSOR_INFO_COLOR_FILTER_ARRANGEMENT_MONO) {
+
+                    if (bestCameraId == null) {


Лучше во всём таком коде сокращать вложенность if-ов, отсекая случаи по одному:

if (cameraCharacteristics.get(CameraCharacteristics.LENS_FACING) != CameraCharacteristics.LENS_FACING_BACK) { continue; } if (cameraCharacteristics.get(CameraCharacteristics.SENSOR_INFO_COLOR_FILTER_ARRANGEMENT) == CameraCharacteristics.SENSOR_INFO_COLOR_FILTER_ARRANGEMENT_MONO) { continue; } if (bestCameraId == null) { bestCameraId = cameraId; continue; } // дальше код с компаратором

krasko · 2021-03-21T03:50:43Z

app/src/main/java/ru/hse/colorshare/ReceiverCameraActivity.java

+                    if (cameraIdByResolutionComparator.compare(bestCameraId, cameraId) > 0) {
+                        bestCameraId = cameraId;
+                    }


Я бы стал его вводить компаратор если хочется использовать метод Collections.max(Collection, Comparator), сейчас же мы всё равно идём в цикле по-старинке, и он только затрудняет чтение, так как можно просто вычислить два разрешения и сравнить, больше одно другого или нет.

Size pixelArraySize = cameraCharacteristics.get(CameraCharacteristics.SENSOR_INFO_PIXEL_ARRAY_SIZE); long resolution = ((long) pixelArraySize.getHeight()) * pixelArraySize.getWidth(); if (resolution > bestResolution) { bestCameraId = cameraId; bestResolution = resolution; }

Это во много раз проще читается (не надо помнить, какая конвенция у компаратора -- когда +1, когда -1, нет лишнего кода, итд), у него лучше производительность (не надо вытаскивать камеру по id), итд

krasko · 2021-03-21T03:52:19Z

app/src/main/java/ru/hse/colorshare/ReceiverCameraActivity.java

+    }
+
+    // returns a camera that 1) is not monochrome 2) is of largest sensor area
+    private String chooseCamera() {


По сути здесь ничего не выбирается ("choose" можно трактовать как будто мы выбираем камеру и включаем, например), лучше назвать getBestCameraId например.

Кстати, возвращать лучше Optional<String>, null это большая проблема в программировании в целом

krasko · 2021-03-21T04:24:31Z

app/src/main/java/ru/hse/colorshare/CameraService.java

+//                CaptureRequest request = builder.build();
+
+                // submit a capture request by creating a session
+                ArrayList<Surface> surfacesList = new ArrayList<>();


В левой части просто List<Surface> (это конвенция в Java - использовать наиболее общий из достаточных интерфейсов)

krasko · 2021-03-21T04:24:50Z

app/src/main/java/ru/hse/colorshare/CameraService.java

+                e.printStackTrace();
+                throw new RuntimeException("camera access exception");
+            }


Тоже бы Toast показать

krasko · 2021-03-21T04:28:39Z

app/src/main/java/ru/hse/colorshare/CameraService.java

+        } else if (scaleByHeight * size.getWidth() < screenSizeRect.width()) {
+            previewView.setLayoutParams(new FrameLayout.LayoutParams((int) (size.getWidth() * scaleByHeight), (int) (size.getHeight() * scaleByHeight)));
+        } else {
+            throw new AssertionError("unscalable size?");


Если мы вычислим a = b/c, то результат a*c может быть немного больше, немного меньше, или равен b. Это же арифметика с плавающей точкой, могут быть погрешности округления. Так что это исключение может начать падать "случайно" (например, и размер и экран квадратные, но результат деления неточный).

krasko · 2021-03-21T04:34:46Z

app/src/main/java/ru/hse/colorshare/CameraService.java

+
+    public void closeCamera() {
+        if (cameraDevice != null) {
+            isCameraOpen.set(false);


Вообще, мне кажется, что весь этот флаг здесь лишний. Он всегда true, когда cameraId выставлен, и всегда false, когда нет.

krasko · 2021-03-21T04:36:00Z

app/src/main/java/ru/hse/colorshare/ImageStreamHandler.java

+    public static ImageStreamHandler getInstance() {
+        if (instance == null) {
+            instance = new ImageStreamHandler();
+        }
+        return instance;
+    }
+
+    private ImageStreamHandler() {


Синглетоны это плохо (ухудшают тестируемость), но это можно обсудить.

krasko · 2021-05-10T06:40:14Z

app/src/main/java/ru/hse/colorshare/ImageProcessor.java

+    private ScriptIntrinsicYuvToRGB script;
+    private Allocation in, out;
+
+    private boolean isInit = false;


krasko · 2021-05-10T06:44:47Z

app/src/main/java/ru/hse/colorshare/CameraService.java

-                        image.close(); // asap
-                        ImageProcessor.process(new ImageProcessor.Task(nv21, width, height, ReceiverCameraActivity.getReadingStatusHandler()));
+                        image.close(); // ASAP
+                        ImageProcessor.getInstance().EXECUTOR.execute(new ImageProcessor.Task(nv21, width, height, ReceiverCameraActivity.getReadingStatusHandler()));


Старый способ мне, кстати, больше нравится по стилю. Метод process может сам сделать getInstance().EXECUTOR.execute.

krasko · 2021-05-10T06:50:58Z

app/src/main/java/ru/hse/colorshare/FrameOverlayView.java

+            double resultH = effectiveH;
+            double marginX = margin;
+            double marginY = margin;
+            if (effectiveH / effectiveW > ImageProcessor.RATIO) {


Маловероятно, но потенциально может быть деление на ноль (очень маленький экран?). В этом случае лучше что-нибудь специальное сделать, показать ошибку например. Вряд ли такое будет конечно, но проверить, что оба числа положительны, не мешает.

krasko · 2021-05-10T06:59:20Z

app/src/main/java/ru/hse/colorshare/ImageProcessor.java

+            if (!ImageProcessor.getInstance().isInit) {
+                ImageProcessor.getInstance().init(width, height, imageBytes.length);


Учитывая, что init() проверяет isInit, и при этом init - synchronized, всё это выглядит как-то не очень надежно. Может быть здесь тоже должна быть синхронизация на мониторе объекта ImageProcessor, а при этом ImageProcessor не должен проверять, что он уже isInit, раз мы тут проверяем?

Кажется, можно сделать более надежный API у ImageProcessor-a, например просто передавать ему в метод yuv420ToBitmap и ширину, и высоту, и байты, и пусть он сам разбирается, надо ли ему делать init; изменяется ли ширина и высота от картинки к картинке. Сейчас, по идее, то, что должно быть приватным делом ImageProcessor-a, проверяет Task - то есть API не очень хорошо абстрагирует внутренности класса.

Ровно так, как Вы говорите, у меня и было сделано! Но потом я подумал какую-то мысль, а сейчас уже не помню, какую, и переделал... Технически изменение размеров кадра может только затормозить работу, но ломаться от этого, кажется, ничего не должно. С другой стороны, меняться эти размеры не должны, и если они будут меняться - что-то идет не так.

krasko · 2021-05-10T07:01:29Z

app/src/main/java/ru/hse/colorshare/ImageProcessor.java

-        executor.shutdown();
-    }
+    private final int poolSize = 5; // ! needs testing
+    public final ExecutorService EXECUTOR = Executors.newFixedThreadPool(poolSize);


Обычно заглавными буквами называют то, что "полностью константно" - строки, числа, может быть объекты иммутабельных классов, но не более. Т.е. это не просто константная ссылка, но и состояние объекта не меняется. У ExecutorService же внутри полно изменяемого состояния.

krasko · 2021-05-10T07:05:31Z

app/src/main/java/ru/hse/colorshare/receiver/ColorExtractor.java

+            Log.d("BBW", "width=" + delegate.getWidth() + " height=" + delegate.getHeight());
+        }
+
+        // false for white, true for black


Может ENUM сделать?

Кажется логичным, но не потеряет ли от этого скорость?

В целом не должно: Enum-значения существуют в единственном экземпляре (это, кстати, один из способов написать синглтон -- объявить Enum с единственным значением INSTANCE), поэтому всякие сравнения Enum-ов это сравнения указателей, а возвращение Enum-a - это возвращение уже готового объекта (то есть, по сути, тоже возвращение указателя без каких-либо аллокаций).

krasko · 2021-05-10T07:06:19Z

app/src/main/java/ru/hse/colorshare/receiver/ColorExtractor.java

+    private final static int prevCenterCheckingEps = 10;
+    private final static int prevCenterUnitEps = 3;
+
+    private final static int checkingEps = 15;
+    private final static int minUnit = 3;
+    private final static int maxUnit = 100; // ! probably too much
+
+    private final static int iSkip = 1;
+    private final static int jSkip = 1;


Тут, кстати, написать всё заглавными буквами как раз будет по Java-конвенциям.

krasko · 2021-05-10T07:07:35Z

app/src/main/java/ru/hse/colorshare/receiver/ColorExtractor.java

+            new Check(0, 0, true), // center black area
+            new Check(1, -1, false), // white area
+            new Check(1, 0, false),
+            new Check(1, 1, false),
+            new Check(-1, -1, false),
+            new Check(-1, 0, false),
+            new Check(-1, 1, false),
+            new Check(0, -1, false),
+            new Check(0, 1, false),
+            new Check(2, -2, true), // outer black area
+            new Check(2, 0, true),
+            new Check(2, 2, true),
+            new Check(-2, -2, true),
+            new Check(-2, 0, true),
+            new Check(-2, 2, true),
+            new Check(0, -2, true),
+            new Check(0, 2, true)


Да, точно ENUM нужен для цвета. Сейчас плохо читается.

krasko · 2021-05-10T07:11:34Z

app/src/main/java/ru/hse/colorshare/receiver/ColorExtractor.java

+            for (int i = Math.max(0, pcx - prevCenterCheckingEps); i < Math.min(img.getWidth(), pcx + prevCenterCheckingEps); i += iSkip) {
+                for (int j = Math.max(0, pcy - prevCenterCheckingEps); j < Math.min(img.getHeight(), pcy + prevCenterCheckingEps); j += jSkip) {
+                    for (int unit = Math.max(1, prevCenterUnit - prevCenterUnitEps); unit < prevCenterUnit + prevCenterUnitEps; unit++) {


Можно, наверное, ещё для оптимальности начинать с прошлых координат искать поочередно в обе стороны, и делать что-то типа поиска в ширину вместо поиска в глубину (т.е. искать в порядке удаления тройки (i,j,unit) от прошлой тройки), но это может быть не существенно. Если и так работает надежно и быстро, оставляйте как есть. Вообще, +1 за идею переиспользовать прошлые координаты.

krasko · 2021-05-10T07:13:17Z

app/src/main/java/ru/hse/colorshare/receiver/ColorExtractor.java

+        for (double x : values) {
+            sum += x;
+        }
+        return sum / values.length;


Если values.length ноль, можно вернуть ноль, а не падать.

krasko · 2021-05-10T07:24:48Z

app/src/main/java/ru/hse/colorshare/receiver/ReceiverCameraActivity.java

-                        && cameraCharacteristics.get(CameraCharacteristics.SENSOR_INFO_COLOR_FILTER_ARRANGEMENT) != CameraCharacteristics.SENSOR_INFO_COLOR_FILTER_ARRANGEMENT_MONO) {
-
+                boolean isFacingBack = cameraCharacteristics.get(CameraCharacteristics.LENS_FACING) == CameraCharacteristics.LENS_FACING_BACK;
+                boolean isNotMono = true;


Старайтесь избавляться от not в именах логических переменных. Я понимаю, чего хочется достичь, но not всегда очень увеличивает когнитивную нагрузку в сложных логических выражениях. В какой-то момент, вам понадобится написать isXXX and !isNotMono и это будет тяжело читать. Можно либо использовать антоним (isColor), либо использовать isMono с противоположным значением и добавлять отрицание, где надо. !isMono читается не особо хуже isNotMono, а вот !isNotMono хуже isMono существенно.

krasko · 2021-05-23T08:28:32Z

app/src/main/java/ru/hse/colorshare/receiver/ColorExtractor.java

        }
+
+        public static boolean roundColorBW(int color) {
+            return Color.red(color) + Color.green(color) + Color.blue(color) <= 128 * 3;


Видимо на данном этапе не сильно критично, но вообще кажется, что при изменении яркости освещения значения будут очень сильно "плавать" - и эта проверка может быть не очень надежной (белый будет иногда считаться черным, а черный белым). Может, как-то нормировать на среднюю яркость картинки вместо константы 128?

krasko · 2021-05-23T08:33:41Z

app/src/main/java/ru/hse/colorshare/receiver/ColorExtractor.java

+        for (int value : values) {
+            sum += value;
+        }
+        int avgRounded = round(sum / 4.0);


Не очень понятно из кода, почему делим на 4. Может, делать на values.length? Если в эту функцию всегда передаются массивы длины 4, то тогда наверное лучше сделать четыре int-аргумента?

krasko · 2021-05-23T08:39:37Z

app/src/main/java/ru/hse/colorshare/receiver/ColorExtractor.java

+        if ((x <= 5 && y <= 5) || (width - x <= 6 && y <= 5) || (width - x <= 6 && height - y <= 6) || (x <= 5 && height - y <= 6)) {
+            // simplified version is unreadable
+            return false;
+        }
+        return true;


Может будет более читаемо, если обработать случаи отдельно:

if (x <= 5 && y <= 5) return true; if (x >= width - 6 && y <= 5) return true; ... return false;

Вообще, правда, что это выражение эквивалентно

if (5 < x && x < width - 6) return true; if (5 < y && y < height - 6) return true; return false;

? Т.е true получаем в "крестообразной" области?

krasko · 2021-05-23T08:47:36Z

app/src/main/java/ru/hse/colorshare/receiver/ColorExtractor.java


        public static boolean roundColorBW(int color) {
-            return Color.red(color) + Color.green(color) + Color.blue(color) <= 128 * 3;
+            return Color.red(color) + Color.green(color) + Color.blue(color) <= 200; // ! 128 * 3 sometimes fails


О, понятно, я как раз про это только что написал на прошлый коммит. Видимо да, надо учитывать, что обычное наше восприятие яркости, оно, во-первых, скорее логарифмическое чем линейное, а во-вторых очень сильно зависит от яркости окружающей области. Но если просто 200 работает, то ок)

krasko · 2021-05-23T08:52:45Z

app/src/main/java/ru/hse/colorshare/receiver/ColorExtractor.java

+//            for (int x = Math.max(0, pcx - PREV_CENTER_CHECKING_EPS); x < Math.min(img.getWidth(), pcx + PREV_CENTER_CHECKING_EPS); x += xSkip) {
+//                for (int y = Math.max(0, pcy - PREV_CENTER_CHECKING_EPS); y < Math.min(img.getHeight(), pcy + PREV_CENTER_CHECKING_EPS); y += ySkip) {
+//                    for (int unit = Math.max(1, prevCenterUnit - PREV_CENTER_UNIT_EPS); unit < prevCenterUnit + PREV_CENTER_UNIT_EPS; unit++) {
+//                        if (checkCenter(img, x, y, unit)) {
+//                            return specifyCenter(img, x, y, unit);
+//                        }
+//                    }
+//                }


Редко необходимо оставлять закомментированный код в репозитории. Если хочется просто иметь возможность посмотреть старые версии кода, то есть история, а если хочется что-то сообщить, то комментарии будут обычно более понятны.

krasko · 2021-05-23T09:03:01Z

app/src/main/java/ru/hse/colorshare/receiver/ColorExtractor.java

-    private static final int maxUnitDifference = 4;
+        int magic = 3 + round(Math.max(0, img.getHeight() - 800) / 2000.0 * 8);
+
+        return new LocatorResult(round(average(right, left)), round(average(top, bottom)),


А как вы сейчас это отлаживаете / как показывается пользователю, что в данный момент происходит? Сейчас смотрю, как работает обычный сканер QR-кодов на айфоне, и он подсвечивает, где он думает, что находятся опорные точки; может что-нибудь такое же сделать?

krasko · 2021-05-23T09:05:36Z

app/src/main/java/ru/hse/colorshare/receiver/ImageProcessor.java

            }
            Bitmap bitmap = ImageProcessor.getInstance().yuv420ToBitmap(imageBytes);
+            imageBytes = null; // help GC!
+//            System.gc(); // ! this might slow everything down


Да, обычно gc сам знает, когда ему приходить.. В любом случае, System.gc() это только рекомендация. imageBytes = null -- разумно.

krasko · 2021-05-23T09:11:06Z

app/src/main/java/ru/hse/colorshare/receiver/ImageProcessor.java

+            lock.lock();
+            super.put(e);
+            lock.unlock();


Вообще, объявление каждого метода как synchronized, на первый взгляд, должно делать то же самое, что и эти локи.

Должно быть возможно (и совершенно нормально) добавлять synchronized в классах-наследниках (но не удалять имеющийся в предках synchronized - это нарушит принцип подстановки Лисков).

krasko · 2021-05-23T09:13:38Z

app/src/main/java/ru/hse/colorshare/receiver/ImageProcessor.java

+
        getInstance().executor.execute(t);
    }



Вообще вся логика довольно интересная, но хотелось бы понять и обсудить (не уверен, что всё в ней понимаю).

krasko

С lastResult надо точно хорошо подумать - это первое, что я нашёл, но обычно в многопоточном приложении бывает ещё много всяких скрытых и неочевидных проблем.

krasko · 2021-05-23T09:31:13Z

app/src/main/java/ru/hse/colorshare/receiver/ColorExtractor.java

-    private static LocatorResult[] lastResult;
+    // found out that threads actually interfere with each other and break sometimes
+
+    private volatile static LocatorResult[] lastResult; // different threads make this 'null' when unnecessary


Ломается это дело скорее всего ниже, в коде:

if (lastResult != null) .... new Point(lastResult[i].x, lastResult[i].y)

Так как код исполняется во многих потоках, то к тому моменту, как вы вычисляете lastResult[i].x, lastResult уже может быть null, даже если раньше он был не-null. Надо быть очень аккуратным с доступом к общим данным из многих потоков.

Проблему можно решить, если сделать так:

LocatorResult[] lastResultLocal = lastResult; if (lastResultLocal != null) .... new Point(lastResultLocal[i].x, lastResultLocal[i].y)

Локальную переменную никто перезаписывать не будет - если получилось схватить не-null последний результат, то он таковым точно останется. Но здесь есть ещё более тонкие моменты, например: если переменная lastResult не объявлена как volatile, то и это может дальше падать (т.к. не гарантируется, что если 2й поток "увидел", что в lastResult что-то записали, то он увидит результаты работы конструкторов отдельных элементов массива). То, как ведут себя чтения и записи переменных в случае многих потоков не подчиняется ожидаемой логике, при которой события происходят в каком-то порядке в "линейном" времени, там скорее частично упорядоченное множество. Кажется с volatile всё работает, но на всякий случай я бы всё равно вместо volatile брал lock на время извлечения общих данных в lastResultLocal. Взять один lock на кадр - не принесет никаких заметных проблем с перформансом.

krasko · 2021-05-23T09:38:36Z

app/src/main/java/ru/hse/colorshare/receiver/ImageProcessor.java

+                try {
+                    super.take();
+                } catch (InterruptedException e) {
+                    // ignored


Можно выйти из цикла. Хотя я не знаю, что сейчас ожидается в случае многих потоков - когда один достает, а второй кладёт. Должен ли первый попробовать достать ещё в случае первой неудачи? Сейчас эта логика, видимо, довольно плохо определена.

kibrq and others added 5 commits March 6, 2021 13:07

Init

bad57c5

Show camera output

208f7a8

Dive into formats and cam params, currently broken

549f51f

Change SurfaceView to TextureView, camera kind of works again

abbc568

Trying to set size, image is still distorted

c9ed986

krasko reviewed Mar 21, 2021

View reviewed changes

DLochmelis33 added 6 commits April 16, 2021 23:02

Finally fix distorted camera image

242a7ee

Reading images from camera

7eea229

Try dummy payload and overlay drawing

4d4aa1e

Convert YUV to RGB

632d8c5

Add overlay for locators position hints (V4)

acc4951

Locator finding V1 + tests

6fc917d

krasko approved these changes May 10, 2021

View reviewed changes

DLochmelis33 added 3 commits May 18, 2021 20:21

Extracting colors real-time!

97c1fc9

A lot of small changes to image parsing

f34262c

Before merge

496d72a

krasko reviewed May 23, 2021

View reviewed changes

krasko approved these changes May 23, 2021

View reviewed changes

		if (!ImageProcessor.getInstance().isInit) {
		ImageProcessor.getInstance().init(width, height, imageBytes.length);

Comments

Conversation

DLochmelis33 commented Mar 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krasko Mar 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krasko May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krasko May 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krasko May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krasko May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krasko Mar 21, 2021 •

edited

Loading

krasko May 10, 2021 •

edited

Loading

krasko May 23, 2021 •

edited

Loading

krasko May 10, 2021 •

edited

Loading

krasko May 10, 2021 •

edited

Loading

krasko May 23, 2021 •

edited

Loading