Conversation
Using the same linter configuration as for egress. Bunch of errors some outlining real bugs. Majority of linter errors fixed: - unused params - depricated APIs - returning unexported types - typos - bug with using TrimLeft - bug with copying mutex (part of copied proto message) - big param list (not really fixed - just disabled for now to avoid big changes)
|
|
||
| err = o.preProcessorElements[len(o.preProcessorElements)-1].Link(o.tee) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Were we hitting this before and ignoring?
There was a problem hiding this comment.
No, if this link failed - the pipeline would be broken, the tee element would have no input. So this will cause gstreamer to error out when pipeline starts (not connected) and produce an error that would be a bit cryptic. I don't see that happening from logs. Realistically this probably never fails. It looks just like forgotten error check. Maybe @biglittlebigben or @frostbyte73 could double check.
There was a problem hiding this comment.
We should definitely add the check, even if unlikely to hit with the current code base. Whether we currently ever hit this and then fail later right now seems a bit academic?
pkg/params/params.go
Outdated
| return path.Join(os.TempDir(), info.State.ResourceId) | ||
| } | ||
|
|
||
| //nolint:revive // argument-limit: refactoring signature would be a larger change |
There was a problem hiding this comment.
maybe make the comment about refactoring as an inline PR comment and make it a TODO in code. Reading that code comment some time after now will feel strange :-) You would have to check blame/history to understand that comment
pkg/params/presets.go
Outdated
|
|
||
| func computeVideoLayers(highLayer *livekit.VideoLayer, layerCount int) []*livekit.VideoLayer { | ||
| layerCopy := *highLayer | ||
| layerCopy := proto.Clone(highLayer).(*livekit.VideoLayer) |
There was a problem hiding this comment.
We have a utils.CloneProto in protocol repo which we have started using as a standard way to clone protobuf. Would be good to see if that is usable here also.
There was a problem hiding this comment.
that's very neat - replacing.
There was a problem hiding this comment.
we have couple more of explicit proto.Clone calls which could be replaced - replacing them as well.
| } | ||
| } | ||
|
|
||
| //nolint:revive // TODO(milos) reduce argument count |
There was a problem hiding this comment.
made a comment earlier, meant to say something like this is better for code comment.
| depacketizer := &codecs.VP8Packet{} | ||
|
|
||
| b, err := depacketizer.Unmarshal(pkt.Payload) | ||
| if err != nil { |
There was a problem hiding this comment.
I was also asking myself this question when I modified this - thanks for bringing it up - I should have explicitly mentioned it - as far as I can see - before this return - the error will be emitted by the decoder (reader with 0 bytes - something like unexpected EOF). Functionally nothing changes now other than hopeful having more precise error message if this ever happens (don't see it happening from logs for the past month).
|
|
||
| err = o.preProcessorElements[len(o.preProcessorElements)-1].Link(o.tee) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
We should definitely add the check, even if unlikely to hit with the current code base. Whether we currently ever hit this and then fail later right now seems a bit academic?
| return path.Join(os.TempDir(), info.State.ResourceId) | ||
| } | ||
|
|
||
| //nolint:revive // TODO(milos): reduce argument count |
There was a problem hiding this comment.
Yeah, this got out of hand 1 argument at a time. That's case where linters are good to keep you in check.
3 participants
Using the same linter configuration as for egress. Bunch of errors some outlining real bugs.
Majority of linter errors fixed: