add motion vector extraction from side data by cvley · Pull Request #33 · imkira/go-libav

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

 	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

 //  }
 //  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

+//	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

+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

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


can you do the same here for NewMotionVectorFromC ?

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

+	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

+	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

+	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

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


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

+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 imkira@gmail.com and I will upload it for the tests.
Thanks

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

+	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

+	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

+		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

+	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

 	}
 }
+
+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

+	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

Conversation

cvley commented Mar 24, 2017

Uh oh!

codecov-io commented Mar 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

imkira commented Mar 26, 2017

Uh oh!

cvley commented Mar 26, 2017

Uh oh!

cvley commented Mar 27, 2017

Uh oh!

imkira commented Mar 28, 2017

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!

imkira commented Jun 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-io commented Mar 24, 2017 •

edited

Loading