Add pixelFormat to GetCodecInfo() #303
Conversation
ffmpeg/ffmpeg_test.go
Outdated
| @@ -1739,7 +1741,8 @@ func TestTranscoder_GetCodecInfo(t *testing.T) { | |||
| t.Errorf("Unexpected error %v", err) | |||
| } | |||
| fname = path.Join(wd, "..", "data", "bunny.mp4") | |||
| res, acodec, vcodec, err = GetCodecInfo(fname) | |||
| res, acodec, vcodec, pixelFormat, err = GetCodecInfo(fname) | |||
| fmt.Printf("bunny.mp4 %t %s %s %d %v\n", res, acodec, vcodec, pixelFormat, err) | |||
There was a problem hiding this comment.
Printf() shall be removed later
AlexKordic
force-pushed
the
ak/pixelformatinfo
branch
2 times, most recently
from
f030ed7
to
72d8fd5
Compare
AlexKordic
requested review from
cyberj0g,
yondonfu and
darkdarkdragon
and removed request for
yondonfu
| // gopFloat is allowed to be 0 | ||
| if gopFloat < 0.0 { |
There was a problem hiding this comment.
Tests in go-livepeer repo are designed to test for 0 in profile.GOP. Fixing this condition to allow 0
There was a problem hiding this comment.
LGTM! Good work @AlexKordic
Please remove Printf() before merging.
ffmpeg/videoprofile.go
Outdated
| P240p30fps4x3 = VideoProfile{Name: "P240p30fps4x3", Bitrate: "600k", Framerate: 30, AspectRatio: "4:3", Resolution: "320x240"} | ||
| P144p30fps16x9 = VideoProfile{Name: "P144p30fps16x9", Bitrate: "400k", Framerate: 30, AspectRatio: "16:9", Resolution: "256x144"} | ||
| P144p25fps16x9 = VideoProfile{Name: "P144p25fps16x9", Bitrate: "400k", Framerate: 25, AspectRatio: "16:9", Resolution: "256x144"} | ||
| P720p60fps16x9 = VideoProfile{Name: "P720p60fps16x9", Bitrate: "6000k", Framerate: 60, AspectRatio: "16:9", Resolution: "1280x720", ChromaFormat: ChromaSubsampling420, ColorDepth: ColorDepth8Bit} |
There was a problem hiding this comment.
Maybe not initialize these fields to default enum values?
There was a problem hiding this comment.
I am not sure to understand yet what is the consequence? What is wanted behavior?
There was a problem hiding this comment.
Oh, I see, ColorDepth is actually number of bits, not enum. To specify default value in this case, we'll need to add a NewVideoProfile function and replace all direct VideoProfile initialization throughout the code. With current implementation, it looks like it may later break some tests which are initializing VideoProfile, because ColorDepth will be 0 without explicit initialization. Alternative is to use usual int enum semantics, where 0 maps to default value (8 bits), and then map it to number of bits in lpms.
What I originally meant probably would still apply to ChromaFormat, we have default value of iota (0) there, which represents ChromaSubsampling420, maybe omit it completely in VideoProfile initializer for the sake of clarity?
There was a problem hiding this comment.
Maybe we could change ColorDepthBits to be ColorDepthBitsMinus8 to make it default 0 being 8bit default?
…own codec. Refactor `lpms_get_codec_info()`.
Export constant for `AV_PIX_FMT_NONE` Define constants for `ColorDepthBits`
Update `lpms_get_codec_info()` description.
AlexKordic
force-pushed
the
ak/pixelformatinfo
branch
from
9b64161
to
5fee68e
Compare
AlexKordic
requested review from
cyberj0g
and removed request for
darkdarkdragon
ffmpeg/ffmpeg.go
Outdated
| func (pixelFormat PixelFormat) Properties() (ChromaSubsampling, ColorDepthBits, error) { | ||
| switch pixelFormat.RawValue { | ||
| case C.AV_PIX_FMT_YUV420P: | ||
| return ChromaSubsampling420, 0, nil |
There was a problem hiding this comment.
Since value doesn't have direct meaning anymore, can we use named constants for ColorDepthBits here?
| type ColorDepthBits int | ||
|
|
||
| const ( | ||
| ColorDepth8Bit ColorDepthBits = 0 |
There was a problem hiding this comment.
Maybe add a comment detailing how these values are related to actual number of bits?
Documented relation of ColorDepthBits value to bit value
No description provided.