add motion vector extraction from side data #33

cvley · 2017-03-24T10:06:51Z

Hi, I need motion vector extraction from video, so I implement related functions.

Pls. review them, thanks 😄

codecov-io · 2017-03-24T10:30:40Z

Codecov Report

Merging #33 into master will decrease coverage by 0.32%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master      #33      +/-   ##
==========================================
- Coverage   32.11%   31.79%   -0.33%     
==========================================
  Files           4        4              
  Lines        2525     2551      +26     
==========================================
  Hits          811      811              
- Misses       1667     1693      +26     
  Partials       47       47

Impacted Files	Coverage Δ
avutil/avutil.go	`31.75% <0%> (-1.17%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a36251d...496bfea. Read the comment docs.

imkira · 2017-03-26T11:43:55Z

avutil/avutil.go

@@ -980,6 +989,64 @@ func (f *Frame) PacketDuration() int64 {
 	return int64(C.av_frame_get_pkt_duration(f.CAVFrame))
 }

+func (f *Frame) GetSideData() *FrameSideData {


In go I like to avoid Get... as much as possible.
Please rename to SideData.
Also, please make it accept the type of side data:

func (f *Frame) SideData(dataType FrameSideDataType) *FrameSideData

Please define them on the top of the file as something like this:

https://github.com/cvley/go-libav/blob/e12c9dd34b37af9a083f8423fb25b6d04d277ce4/avutil/avutil.go#L104-L115

Please use these values:

https://www.ffmpeg.org/doxygen/3.0/frame_8h_source.html#l00048

type FrameSideDataType C.enum_AVFrameSideDataType const ( FrameSideDataTypePanScan FrameSideDataType = C.AV_FRAME_DATA_PANSCAN ... FrameSideDataTypeMotionVectors FrameSideDataType = C.AV_FRAME_DATA_MOTION_VECTORS ... )

imkira · 2017-03-26T11:44:41Z

avutil/avutil.go

@@ -41,6 +42,14 @@ package avutil
 //  }
 //  return 0;
 //}
+//static const AVMotionVector *go_motion_vector(const AVFrameSideData *sd, int num) {


An extra line here between functions please.

imkira · 2017-03-26T11:45:03Z

avutil/avutil.go

+//	const AVMotionVector *mvs = (const AVMotionVector *)sd->data;
+//	return &mvs[num];
+//}
+//static int go_motion_vector_size(const AVFrameSideData *sd) {


imkira · 2017-03-26T11:47:13Z

avutil/avutil.go

+type FrameSideData struct {
+	CFrameSideData *C.AVFrameSideData
+}
+


Since I am following this pattern, can you follow it too?

func NewFrameSideDataFromC(cFrameSideData unsafe.Pointer) * FrameSideData { return &FrameSideData{CFrameSideData: (*C.AVFrameSideData)(cFrameSideData)} }

and then call it inside Frame.SideData() ?

imkira · 2017-03-26T11:53:49Z

avutil/avutil.go

+type MotionVector struct {
+	CMotionVector *C.AVMotionVector
+}
+


can you do the same here for NewMotionVectorFromC ?

imkira · 2017-03-26T12:16:50Z

avutil/avutil.go

+	return int64(mv.CMotionVector.source)
+}
+
+func (mv *MotionVector) Width() int64 {


Any reason why you use int64?
libav returns int8 for this: https://www.ffmpeg.org/doxygen/3.0/structAVMotionVector.html#ae208462700b6b52d22ed38e9d62cf2cf
I think we should use int8 too. Same for Height

imkira · 2017-03-26T12:18:20Z

avutil/avutil.go

+	return int64(mv.CMotionVector.dst_x), int64(mv.CMotionVector.dst_y)
+}
+
+func (mv *MotionVector) MotionVector() (int64, int64) {


Since this is a MotionVector we should just call this Vector
libav defines this as int32.

imkira · 2017-03-26T12:18:46Z

avutil/avutil.go

+	return int64(mv.CMotionVector.motion_x), int64(mv.CMotionVector.motion_y)
+}
+
+func (mv *MotionVector) MotionScale() int64 {


Since this is a MotionVector we should just call this Scale
libav defines this as uint16.

imkira · 2017-03-26T12:19:33Z

avutil/avutil.go

+	CMotionVector *C.AVMotionVector
+}
+
+func (mv *MotionVector) Source() int64 {


imkira · 2017-03-26T12:20:54Z

avutil/avutil.go

+func (mv *MotionVector) MotionScale() int64 {
+	return int64(mv.CMotionVector.motion_scale)
+}
+


Please add support for Flags too:

https://www.ffmpeg.org/doxygen/3.0/structAVMotionVector.html#a75d00ba82df36ec5e697527ddb9addcc

func (mv *MotionVector) Flags() uint64 { return mv.CMotionVector.flags }

imkira · 2017-03-26T12:22:20Z

@cvley Thank you for your PR.
Please take a look at the review.
Can you also add some simple tests where you call all new functions?
Thanks

cvley · 2017-03-26T13:17:51Z

@imkira Thanks for your reviews. I will work on it 😄

cvley · 2017-03-27T23:36:56Z

@imkira I have fixed the code and add a simple test, please review them.

By the way, I'd like to generate a random video in the test to perform motion vectors extraction, is it OK?

Thanks.

imkira · 2017-03-28T13:43:28Z

@cvley No problem about the random video.

During tests I'm currently downloading to a directory called fixtures

https://bintray.com/imkira/go-libav/download_file?file_path=sample_iPod.m4v
https://bintray.com/imkira/go-libav/download_file?file_path=sample_iTunes.mov
https://bintray.com/imkira/go-libav/download_file?file_path=sample_mpeg4.mp4

And referencing them like this:

go-libav/avformat/avformat_test.go

Line 60 in 126f4da

fixture := fixturePath("sample_mpeg4.mp4")

Do any of these work for your tests?

If not, please send me the file to [email protected] and I will upload it for the tests.
Thanks

imkira · 2017-03-28T13:44:07Z

avutil/avutil.go

+	FrameDataPanScan                  FrameSideDataType = C.AV_FRAME_DATA_PANSCAN
+	FrameDataA53CC                    FrameSideDataType = C.AV_FRAME_DATA_A53_CC
+	FrameDataStereo3D                 FrameSideDataType = C.AV_FRAME_DATA_STEREO3D
+	FrameDataMatrixEncoding           FrameSideDataType = C.AV_FRAME_DATA_MATRIXENCODING


Maybe we should use _ too in MATRIX_ENCODING

Do you mean we use FrameDataMATRIX_ENCODING instead of FrameDataMatrixEncoding?

Should I change all these types using _?

Thanks.

imkira · 2017-03-28T13:44:27Z

avutil/avutil.go

+	FrameDataMatrixEncoding           FrameSideDataType = C.AV_FRAME_DATA_MATRIXENCODING
+	FrameDataDownMixInfo              FrameSideDataType = C.AV_FRAME_DATA_DOWNMIX_INFO
+	FrameDataReplayGain               FrameSideDataType = C.AV_FRAME_DATA_REPLAYGAIN
+	FrameDataDisplayMatrix            FrameSideDataType = C.AV_FRAME_DATA_DISPLAYMATRIX


DISPLAY_MATRIX

imkira · 2017-03-28T13:46:24Z

avutil/avutil.go

+		return
+	}
+
+	C.av_buffer_unref(unsafe.Pointer(&sd.CFrameSideData.buf))


Aren't these managed pointers. Do we to provide this?

We don't use buf directly here, but we need to free it.

I will dig more about the buf and av_buffer_unref usage.

Thanks.

imkira · 2017-03-28T13:48:11Z

avutil/avutil.go

+	return FrameSideDataType(sd.CFrameSideData._type)
+}
+
+func (sd *FrameSideData) MotionVector(n int) *MotionVector {


I see you didn't follow my advice for MotionVectors() []*MotionVector.
Also, you're not using go_motion_vector_size anymore.
How will you discover the number of motion vectors in go?

Sorry about this

imkira · 2017-03-28T13:48:56Z

avutil/avutil_test.go

@@ -845,3 +845,13 @@ func TestPixelFormatDescriptor_ComponentCount(t *testing.T) {
 		t.Fatalf("[TestPixelFormatDescriptor_ComponentCount] count=%d, NG expected=%d", count, 3)
 	}
 }
+
+func TestMotionVectorFromFrameSideData(t *testing.T) {


Can you make a separate function TestFrameSideData where you call all its funcs?

No problems. I will try your test videos and random generated videos. 😄

imkira · 2017-03-28T13:49:06Z

avutil/avutil_test.go

+	sideData := frame.SideData(FrameDataMotionVectors)
+	if sideData != nil {
+		t.Fatalf("[TestMotionVectorFromFrameSideData] sidedata=%v, NG expected=nil", sideData)
+	}


Can you call all MotionVector's funcs?

No problems.

imkira · 2017-06-30T02:36:00Z

Any update on this @cvley ?

imkira requested changes Mar 26, 2017

View reviewed changes

add motion vector extraction from side data

496bfea

cvley force-pushed the master branch from d97967c to 496bfea Compare March 27, 2017 23:31

imkira requested changes Mar 28, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add motion vector extraction from side data #33

add motion vector extraction from side data #33

cvley commented Mar 24, 2017

codecov-io commented Mar 24, 2017 •

edited

Loading

imkira Mar 26, 2017

imkira Mar 26, 2017

imkira Mar 26, 2017

imkira Mar 26, 2017

imkira Mar 26, 2017

imkira Mar 26, 2017

imkira Mar 26, 2017

imkira Mar 26, 2017

imkira Mar 26, 2017

imkira Mar 26, 2017

imkira commented Mar 26, 2017

cvley commented Mar 26, 2017

cvley commented Mar 27, 2017

imkira commented Mar 28, 2017

imkira Mar 28, 2017

cvley Mar 30, 2017

imkira Mar 28, 2017

imkira Mar 28, 2017

cvley Mar 30, 2017

imkira Mar 28, 2017

cvley Mar 30, 2017

imkira Mar 28, 2017

cvley Mar 30, 2017

imkira Mar 28, 2017

cvley Mar 30, 2017

imkira commented Jun 30, 2017

add motion vector extraction from side data #33

Are you sure you want to change the base?

add motion vector extraction from side data #33

Conversation

cvley commented Mar 24, 2017

codecov-io commented Mar 24, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imkira commented Mar 26, 2017

cvley commented Mar 26, 2017

cvley commented Mar 27, 2017

imkira commented Mar 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imkira commented Jun 30, 2017

codecov-io commented Mar 24, 2017 •

edited

Loading