http push: return segment unchanged if it contains zero video frames

[darkdarkdragon]

What does this pull request do? Explain your changes. (required)
Shortcuts HTTP push for segments that contains zero video frame - just returns same segment without transcoding attempt.

Specific updates (required)

• Check every segment in HTTP push handler for 0-fram.
• If so, just returns it.

How did you test each of these updates (required)
unit test

Does this pull request close any open issues?
Fixes #1928

Checklist:

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

[yondonfu]

IIUC the requirements that this PR needs to fulfill are:

• Do not save a source segment with 0 video frames to the record OS (we currently already do this)
• Use the source segment for all renditions in the HTTP push response if the source segment has 0 video frames (we currently return an error in the HTTP push handler)

Given these requirements, could an alternative implementation be:

• Check if the source segment has 0 video frames in processSegment instead of in the HTTP push handler
• If the source segment has 0 video frames, do not save the source segment to the record store
• If the source segment has 0 video frames, processSegment saves each rendition using the source segment data which yield a URL. The URL is used to insert the rendition (which is the source segment) into the playlist. processSegment will then return a list of URLs for the renditions (which all use the source segment data)

Here is a code sketch:

diff --git a/server/broadcast.go b/server/broadcast.go
index 0c6fe912..8a5ea349 100644
--- a/server/broadcast.go
+++ b/server/broadcast.go
@@ -386,22 +386,30 @@ func processSegment(cxn *rtmpConnection, seg *stream.HLSSegment) ([]string, erro
 	name := fmt.Sprintf("%s/%d%s", vProfile.Name, seg.SeqNo, ext)
 	ros := cpl.GetRecordOSSession()
 	segDurMs := getSegDurMsString(seg)
-	if ros != nil {
+
+	// Wrap ffmpeg helper to return boolean
+	// TODO: Return boolean from ffmpeg helper so we don't need this wrapper function
+	hasZeroVideoFrameBytes := func(data []byte) (bool, error) {
+		res, err := ffmpeg.HasZeroVideoFrameBytes(seg.Data)
+		if err != nil {
+			return false, err
+		}
+
+		if res == 1 {
+			return true, nil
+		}
+
+		return false, nil
+	}
+
+	hasZeroVideoFrame, err := hasZeroVideoFrameBytes(seg.Data)
+	if err != nil {
+		// ...
+	}
+
+	if ros != nil && !hasZeroVideoFrame {
 		go func() {
 			now := time.Now()
-			hasZeroVideoFrame, err := ffmpeg.HasZeroVideoFrameBytes(seg.Data)
-			if err != nil {
-				glog.Warningf("Error checking for zero video frame nonce=%d manifestID=%s name=%s bytes=%d took=%s err=%v",
-					nonce, mid, name, len(seg.Data), time.Since(now), err)
-			} else {
-				if hasZeroVideoFrame == 1 {
-					glog.Infof("Checking for zero video frame nonce=%d manifestID=%s name=%s bytes=%d took=%s returned res=%v",
-						nonce, mid, name, len(seg.Data), time.Since(now), hasZeroVideoFrame)
-					// segment has zero video frame, skipping
-					return
-				}
-			}
-			now = time.Now()
 			uri, err := drivers.SaveRetried(ros, name, seg.Data, map[string]string{"duration": segDurMs}, 2)
 			took := time.Since(now)
 			if err != nil {
@@ -440,6 +448,31 @@ func processSegment(cxn *rtmpConnection, seg *stream.HLSSegment) ([]string, erro
 		}
 	}
 
+	// If source segment has 0 video frames, use the source segment for all renditions
+	if hasZeroVideoFrame {
+		urls := make([]string, len(cxn.params.Profiles))
+		for i, profile := range cxn.params.Profiles {
+			ext, err := common.ProfileFormatExtension(profile.Format)
+			if err != nil {
+				// ...
+			}
+
+			name := fmt.Sprintf("%s/%d%s", profile.Name, seg.SeqNo, ext)
+			uri, err := cpl.GetOSSession().SaveData(name, seg.Data, nil)
+			if err != nil {
+				// ...
+			}
+
+			urls[i] = uri
+
+			if err := cpl.InsertHLSSegment(&profile, seg.SeqNo, uri, seg.Duration); err != nil {
+				// ...
+			}
+		}
+
+		return urls, nil
+	}
+
 	var sv *verification.SegmentVerifier
 	if Policy != nil {
 		sv = verification.NewSegmentVerifier(Policy)

I think this alternative implementation would be a little easier to follow and also has the property that rendition data is saved to the OS and inserted into the playlist (although just using the source segment data). I'm not sure how important the latter is, but that property seems like a nice-to-have in that the 0 video frame segment case is almost identical to the non-zero video frame segment case except for the fact that the rendition data = source segment data.

WDYT?

[darkdarkdragon]

@yondonfu changed

[yondonfu]

LGTM!

Can we update the commit messages with the following format (which we've been fairly consistent with in this repo inspired by this) before merging?

<PACKAGE_NAMES>: <DESCRIPTION>

So, a commit message following this format for this PR would be something like:

server: Return 0 video frame segments unchanged