h264: using consts in MbPartPredMode function #25
Conversation
| errPartition = errors.New("partition must be 0") | ||
| ) | ||
|
|
||
| func MbPartPredMode(data *SliceData, sliceType string, mbType, partition int) (mbPartPredMode, error) { |
There was a problem hiding this comment.
Please don't make exported functions return unexported types.
There was a problem hiding this comment.
The function was already exported, and thought changing it would have been out of scope of the PR.
There was a problem hiding this comment.
Sure, but the return type could have been exported.
| } | ||
| return modeName | ||
| return -1, errPartition |
| @@ -20,11 +20,16 @@ import ( | |||
| "github.com/pkg/errors" | |||
| ) | |||
|
|
|||
| type macroblockPredictionMode uint8 | |||
| type mbPartPredMode int8 | |||
There was a problem hiding this comment.
Is there a reason for reducing the length of this name?
There was a problem hiding this comment.
I'm trying to keep the naming more consistent with the specs.
There was a problem hiding this comment.
Clarity is more important than matching the spec; if matching the spec helps, do it, otherwise improve on it. A good example is the crypto/tls package and its constants/field names compared with the relevant RFCs.
There was a problem hiding this comment.
I really think matching the specs as much as possible (unless it breaks go style) is good. It helps to find things in the spec more easily.
There was a problem hiding this comment.
Okay, i'll move away from that idea. Please take a look at the code and create issues if you see naming that could be better.
| modeName := "UnknownPartPredMode" | ||
| // Errors used by MbPartPredMode. | ||
| var ( | ||
| errNaMode = errors.New("no mode for given slice and mb type") |
There was a problem hiding this comment.
what kind of comments do they need ?
There was a problem hiding this comment.
Explaining what they mean?
|
|
||
| const ( | ||
| intra4x4 macroblockPredictionMode = iota | ||
| intra4x4 mbPartPredMode = iota |
There was a problem hiding this comment.
I think these names could be better.
Resolves #14
Replacing usage of strings to represent macroblock partition prediction modes with int8 consts. Also made changes to affected code.