Create avi container for mjpeg by allnes · Pull Request #10145 · opencv/opencv

allnes · 2017-11-23T12:52:47Z

No description provided.

mshabunin · 2017-12-13T10:27:33Z

modules/videoio/src/cap_mjpeg_decoder.cpp


-            AviMjpegStream mjpeg_video_stream;
-            bool is_parsed = mjpeg_video_stream.parseAvi(in_str, m_mjpeg_frames);
+            AVIReadContainer<MjpegInputStream> mjpeg_video_stream;


It will be better to move MjpegInputStream to the same place where AVIReadContainer is located, then you will be able to avoid templates in the code.

@mshabunin And unless the task was not to separate mjpeg codec and avi container, it turns out they will be together to one file again

I don't see a problem here: MjpegInputStream is just a convenient object for reading AVI files, it does not implement MJPEG specific behavior.

@mshabunin I already remade, you can see

mshabunin · 2017-12-13T21:44:13Z

modules/videoio/src/cap_avi_container.cpp

+        AviStreamHeader strm_hdr;
+        *in_str >> strm_hdr;
+
+        if(strm_hdr.fccType == VIDS_CC && strm_hdr.fccHandler == MJPG_CC)


Probably we should pass FourCC code (MJPG_CC here) as a parameter into this method.

@mshabunin corrected

mshabunin · 2018-01-11T14:25:18Z

modules/videoio/src/cap_mjpeg_decoder.cpp



-bool MotionJpegCapture::parseRiff(MjpegInputStream& in_str)
+bool MotionJpegCapture::parseRiff()


I think this method can be moved to the AVIReadContainer class.

mshabunin · 2018-01-11T14:25:33Z

modules/videoio/src/cap_mjpeg_decoder.cpp

@@ -781,17 +140,17 @@ double MotionJpegCapture::getProperty(int property) const

 std::vector<char> MotionJpegCapture::readFrame(frame_iterator it)


I think this method can be moved to the AVIReadContainer class.

mshabunin · 2018-01-11T14:37:21Z

modules/videoio/src/cap_mjpeg_decoder.cpp

 void MotionJpegCapture::close()
 {
-    m_file_stream.close();
+    m_avi_container.getStream()->close();


It is better to avoid exposing internal stream. You can either provide AVIReadContainer::release method or, even better, to create new m_avi_container object for each opened file (store it in Ptr):

Ptr<AVIReadContainer> avi; ... MotionJpegCapture::MotionJpegCapture(const String& filename) { avi = makePtr<AVIReadContainer>(filename); ... } bool MotionJpegCapture::open(const String& filename) { avi = makePtr<AVIReadContainer>(filename); ... }

mshabunin · 2018-01-11T14:56:13Z

modules/core/include/opencv2/core/private.cap_avi_container.hpp

@@ -0,0 +1,479 @@
+/*M///////////////////////////////////////////////////////////////////////////////////////


Let's use short license in all new files:

// This file is part of OpenCV project. // It is subject to the license terms in the LICENSE file found in the top-level directory // of this distribution and at http://opencv.org/license.html.

mshabunin · 2018-01-11T14:58:14Z

modules/videoio/CMakeLists.txt

    ${CMAKE_CURRENT_LIST_DIR}/src/cap_images.cpp
    ${CMAKE_CURRENT_LIST_DIR}/src/cap_mjpeg_encoder.cpp
    ${CMAKE_CURRENT_LIST_DIR}/src/cap_mjpeg_decoder.cpp
+    ${CMAKE_CURRENT_LIST_DIR}/src/cap_avi_container.cpp


Let's rename cap_avi_container.cpp to container_avi.cpp.

mshabunin · 2018-01-11T15:00:48Z

modules/videoio/src/cap_mjpeg_decoder.cpp

 #include "precomp.hpp"
-#include <deque>
-#include <stdint.h>
+#include "opencv2/core/private.cap_avi_container.hpp"


Let's rename this header to container_avi.private.hpp and it should be moved to the videoio module.

mshabunin · 2018-01-11T15:02:04Z

modules/core/include/opencv2/core/private.cap_avi_container.hpp

+    bool       m_is_indx_present;
+};
+
+static const int AVIH_STRH_SIZE = 56;


All variables and functions marked static should be moved to the .cpp file.

mshabunin · 2018-01-11T15:04:24Z

modules/videoio/test/test_avi_container.cpp

+
+#include "test_precomp.hpp"
+#include "opencv2/core/private.cap_avi_container.hpp"
+#include "opencv2/highgui.hpp"


This include should be removed.

mshabunin · 2018-02-07T11:38:27Z

@allnes , we have updated test writing guidelines, please take a look: https://github.com/opencv/opencv/wiki/Coding_Style_Guide#implementing-tests

You have to wrap your test into namespace opencv_test.

mshabunin · 2018-02-09T14:29:25Z

modules/videoio/test/test_container_avi.cpp

+
+using namespace cv;
+using namespace std;
+using namespace opencv_test;


All source code should be put into this namespace.

mshabunin · 2018-02-09T14:29:52Z

modules/videoio/test/test_container_avi.cpp

+#include <cstdio>
+
+using namespace cv;
+using namespace std;


Avoid using namespace std. Use std:: if necessary (common types are imported into opencv_test namespace).

mshabunin · 2018-02-09T14:39:19Z

modules/videoio/test/test_precomp.hpp

+class BunnyParameters
+{
+private:
+    int bunny_width, bunny_height, bunny_fps, bunny_count;


It is not necessary to store values in the object:

class BunnyParameters { public: inline static int getWidth() { return 672; } inline static getCount() { return cvRound(getFps() * getTime()); } ... // call BunnyParameters::getCount()

Filename generation can be provided by this class too:

inline static String getFilename(const String &ext) { return cvtest::TS::ptr()->get_data_path() + "video/big_buck_bunny" + ext; }

allnes · 2018-02-09T17:10:47Z

@mshabunin I'm fixed

mshabunin · 2018-02-13T15:55:14Z

modules/videoio/test/test_video_io.cpp

        ext = get<0>(GetParam());
        apiPref = get<1>(GetParam());
-
        video_file = cvtest::TS::ptr()->get_data_path() + "video/big_buck_bunny." + ext;


Please use bunny_param.getFilename(String(".") + ext) here.

mshabunin · 2018-02-13T15:55:57Z

modules/videoio/test/test_container_avi.cpp

+{
+
+TEST(videoio_avi, good_MJPG) {
+    String filename = cvtest::TS::ptr()->get_data_path() + "video/big_buck_bunny.mjpg.avi";


You should add BunnyParameters::getFilename(".mjpg.avi"); here for consistency.

allnes · 2018-02-14T09:30:49Z

@mshabunin fixed

alalek · 2018-02-14T18:40:46Z

modules/videoio/include/opencv2/videoio/container_avi.private.hpp

+#include "opencv2/core/cvdef.h"
+#include "opencv2/videoio/videoio_c.h"
+#include <deque>
+#include <stdint.h>


This is C++11/C11 header: http://www.cplusplus.com/reference/cstdint/ like uint32_t and other types.

allnes · 2018-02-15T11:45:50Z

@alalek fixed types, review please

mshabunin reviewed Dec 13, 2017

View reviewed changes

mshabunin reviewed Jan 11, 2018

View reviewed changes

mshabunin self-assigned this Jan 25, 2018

mshabunin reviewed Feb 9, 2018

View reviewed changes

mshabunin reviewed Feb 13, 2018

View reviewed changes

mshabunin approved these changes Feb 14, 2018

View reviewed changes

alalek reviewed Feb 14, 2018

View reviewed changes

Added avi-container with tests

df8f618

opencv-pushbot merged commit df8f618 into opencv:master Feb 15, 2018

opencv-pushbot pushed a commit that referenced this pull request Feb 15, 2018

Merge pull request #10145 from allnes:vedeoio_avi_container

30b33a4

allnes deleted the vedeoio_avi_container branch February 15, 2018 15:23

alalek mentioned this pull request Feb 15, 2018

core: build fails on Visual Studio 2013 and earlier #10855

Closed

kikaxa mentioned this pull request Jul 7, 2021

fix videoio/src/container_avi.cpp VideoInputStream alignment crashes on 32bit arm (android) #20373

Merged

6 tasks



		bool MotionJpegCapture::parseRiff(MjpegInputStream& in_str)
		bool MotionJpegCapture::parseRiff()

		@@ -781,17 +140,17 @@ double MotionJpegCapture::getProperty(int property) const

		std::vector<char> MotionJpegCapture::readFrame(frame_iterator it)

		@@ -0,0 +1,479 @@
		/*M///////////////////////////////////////////////////////////////////////////////////////

Uh oh!

Conversation

allnes commented Nov 23, 2017 • edited by mshabunin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

mshabunin commented Feb 7, 2018

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!

allnes commented Feb 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allnes commented Feb 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allnes commented Feb 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

allnes commented Nov 23, 2017 •

edited by mshabunin

Loading