server: fix OT error handling

[reubenr0d]

What does this pull request do? Explain your changes. (required)

Specific updates (required)

• Moved check for transcode errors on O before decodingPixels
• Added nil check for chanData.TranscodeData

How did you test each of these updates (required)

• Tested with errored out video segments, one without a keyframe, and another without Video
• Observed that proper error messages now show on the O
• Observed that EOF is not triggered in case of these errors

Does this pull request close any open issues?

Fixes #1843
Fixes #1762

Checklist:

• Read the contribution guide
• make runs successfully
• All tests in ./test.sh pass
• README and other documentation updated
• Pending changelog updated

[kyriediculous]

I like the solution, it clearly returns the error that should be returned.

Let's also make sure the header is always a sane value. Currently when we encounter a transcoding error from the T

go-livepeer/server/ot_rpc.go

Line 149 in 48409df

tData, err := n.Transcoder.Transcode(md)

We will not set the "Pixels" header

go-livepeer/server/ot_rpc.go

Line 190 in 48409df

if tData != nil {

We should always be setting the "Pixels" header, even if tData is nil and an error is encountered.

If this header is not set we will run into an error converting the string to an integer.

At L190 in ot_rpc.go let's make this change

pixels := int64(0)
    if tData != nil {
        pixels = tData.Pixels
    }
    req.Header.Set("Pixels", strconv.FormatInt(pixels, 10))

---

I'm also wondering if we can add unit tests for this, we can create erroneous responses like this:

		body.Write([]byte(err.Error()))
   	contentType = transcodingErrorMimeType

See

go-livepeer/server/ot_rpc.go

Line 156 in 48409df

body.Write([]byte(err.Error()))

[kyriediculous]

Nevermind on the unit tests, this unit tests should already take care of those cases:

go-livepeer/server/ot_rpc_test.go

Line 178 in 48212dd

func TestRemoteTranscoderError(t *testing.T) {

[kyriediculous]

LGTM 👍

@yondonfu any remarks ? We have tested this both seperately and came to the same conclusions.

No source:

E0504 11:50:45.678492   78423 orchestrator.go:553] Error transcoding manifestID=test sessionID=53e9ccc9 segNo=1 segName=https://127.0.0.1:8935/stream/53e9ccc9/1.tempfile - TranscoderInvalidVideo

No keyframe

E0504 11:48:54.873341   78423 orchestrator.go:553] Error transcoding manifestID=test sessionID=0e05dfc6 segNo=1 segName=https://127.0.0.1:8935/stream/0e05dfc6/1.tempfile - No keyframes in input

[kyriediculous]

I know we didn't touch this in this PR but

https://github.com/livepeer/go-livepeer/blob/master/core/orchestrator.go#L553

This log statement still uses a dash to seperate the error from the rest of the message. We currently use label=..

do you think we can change the end of this statement to err=%v to match this idiom ?

[reubenr0d]

do you think we can change the end of this statement to err=%v to match this idiom ?

Yea, sure. Done.

[kyriediculous]

Looks good to me, let's rebase (squash the fixup)

https://thoughtbot.com/blog/autosquashing-git-commits

[yondonfu]

Looks good to me, but I'm still looking into enabling CI runs for PRs based off of forks (thought I got it working, but looks like some additional tweaks are needed). Will approve & merge once we get CI running & passing.

[reubenr0d]

Looks good to me, but I'm still looking into enabling CI runs for PRs based off of forks (thought I got it working, but looks like some additional tweaks are needed). Will approve & merge once we get CI running & passing.

Ahh, seems to me like the dockerhub creds aren't getting picked up from the Github Secrets over here. Maybe it would be a good idea to not do the docker push for MRs, but only on master merge and only do the build/tests locally for PRs? Let me see what I can do tomorrow.