Skip to contentSkip to conversation

hdpvr: code cleanup

PATCH In review Jonathan Sims 3 participants 3 messages Feb 15, 2017 – Mar 6, 2017

Subsystems (2)
Media (V4L/DVB) Hdpvr USB video encoder driver
lore
Share & tools
lore

Jonathan Sims

author
#0 lore raw

PATCH 1/1 hdpvr: code cleanup

This is a code cleanup after recent changes introduced by commit a503ff812430e104f591287b512aa4e3a83f20b1.

Signed-off-by Jonathan Sims <[email protected]>

---

 drivers/media/usb/hdpvr/hdpvr-video.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)
drivers/media/usb/hdpvr/hdpvr-video.c +1 -1
index 7fb036d6a86e..b2ce5c0807fb 100644
@@ -449,7 +449,7 @@ static ssize_t hdpvr_read(struct file *file, char __user *buffer, size_t count,
 
 		if (buf->status != BUFSTAT_READY &&
 		    dev->status != STATUS_DISCONNECTED) {
-			int err;
+
 			/* return nonblocking */
 			if (file->f_flags & O_NONBLOCK) {
 				if (!ret)

@@ -457,23 +457,19 @@ static ssize_t hdpvr_read(struct file *file, char

__user *buffer, size_t count, goto err;
}

- err =
wait_event_interruptible_timeout(dev->wait_data,
+ ret =
wait_event_interruptible_timeout(dev->wait_data, buf->status == BUFSTAT_READY, msecs_to_jiffies(1000));
- if (err < 0) {
- ret = err;
+ if (ret < 0)
goto err;
- }
- if (!err) {
+ if (!ret) {
v4l2_dbg(MSG_INFO, hdpvr_debug,
&dev->v4l2_dev, "timeout: restart streaming\n");
hdpvr_stop_streaming(dev);
- msecs_to_jiffies(4000);
- err = hdpvr_start_streaming(dev);
- if (err) {
- ret = err;
+ msleep(4000);
+ ret = hdpvr_start_streaming(dev);
+ if (ret)
goto err;
- }
}
}

Signature · 1 line
2.11.1
#1 lore raw
Quoted 42 lines · Jonathan Sims

This is a code cleanup after recent changes introduced by commit a503ff812430e104f591287b512aa4e3a83f20b1.

Signed-off-by Jonathan Sims <[email protected]>

---

 drivers/media/usb/hdpvr/hdpvr-video.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)
drivers/media/usb/hdpvr/hdpvr-video.c +2 -2
index 7fb036d6a86e..b2ce5c0807fb 100644
@@ -449,7 +449,7 @@ static ssize_t hdpvr_read(struct file *file, char __user *buffer, size_t count,
 
 		if (buf->status != BUFSTAT_READY &&
 		    dev->status != STATUS_DISCONNECTED) {
-			int err;
+
 			/* return nonblocking */
 			if (file->f_flags & O_NONBLOCK) {
 				if (!ret)

@@ -457,23 +457,19 @@ static ssize_t hdpvr_read(struct file *file, char
 __user *buffer, size_t count, goto err;
 			}
 
-			err =
 wait_event_interruptible_timeout(dev->wait_data,
+			ret =

wait_event_interruptible_timeout(dev->wait_data, buf->status == BUFSTAT_READY, msecs_to_jiffies(1000));

drivers/media/usb/hdpvr/hdpvr-video.c +2 -4
-			if (err < 0) {
-				ret = err;
+			if (ret < 0)
 				goto err;
-			}
-			if (!err) {
+			if (!ret) {
 				v4l2_dbg(MSG_INFO, hdpvr_debug,

&dev->v4l2_dev, "timeout: restart streaming\n");

drivers/media/usb/hdpvr/hdpvr-video.c +0 -1
 				hdpvr_stop_streaming(dev);
-				msecs_to_jiffies(4000);

drivers/media/usb/hdpvr/hdpvr-video.c

Quoted 10 lines
drivers/media/usb/hdpvr/hdpvr-video.c +3 -4
-				err = hdpvr_start_streaming(dev);
-				if (err) {
-					ret = err;
+				msleep(4000);
+				ret = hdpvr_start_streaming(dev);
+				if (ret)
 					goto err;
-				}
 			}
 		}

One comment and one suggestion:

I believe that the overall solution proposed above is an appropriate means of dealing with hardware/firmware issues in the HD-PVR that lead to failed captures reported by many people. It will be a definite improvement over the current situation. However, it is important to note that some player applications may have minor problems with the resulting mpeg stream. Restarting HD-PVR streaming results in the equivalent of concatenating two separate mpeg streams into one file. Each segment has its own mpeg frame timestamps starting at 0. ffmpeg based applications don't fully handle such files. For example, both mplayer and vlc will play these streams correctly, but both have issues attempting to skip within the stream (vlc skips little or not at all, mplayer will skip but in smaller increments than normal). ffprobe of such a file will report the mpeg length as that of the last capture segment rather than the total length of the file. Still, having a nearly complete capture (absent a few seconds during the restart) with skip problems is far better than having a capture that stops well short of the intended length.

Suggestion: Rather than just having a v4l2 debug message that streaming was restarted, I think it would be desirable to have a timestamped printk message showing the restart and the name of the HD-PVR device (should the system have more than one). This would allow users to more readily tell that they have had this condition occur and how frequently.

Keith

#2 lore raw
In reply to Jonathan Sims

Em Tue, 14 Feb 2017 20:18:32 -0500
Jonathan Sims <[email protected]> escreveu:

Quoted 1 line

This is a code cleanup after recent changes introduced by commit a503ff812430e104f591287b512aa4e3a83f20b1.

Patch doesn't apply:

patch -p1 -i patches/lmml_39419_1_1_hdpvr_code_cleanup.patch --dry-run -t -N checking file drivers/media/usb/hdpvr/hdpvr-video.c
patch: **** malformed patch at line 35: __user *buffer, size_t count, goto err;

Patch may be line wrapped
checking file drivers/media/usb/hdpvr/hdpvr-video.c
patch: **** unexpected end of file in patch

Your e-mailer is breaking long lines, causing it to not work.

Please either configure your e-mailer to not wrap long lines or use git to send it.

Quoted 41 lines
Signed-off-by Jonathan Sims <[email protected]>

---

 drivers/media/usb/hdpvr/hdpvr-video.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)
drivers/media/usb/hdpvr/hdpvr-video.c +2 -2
index 7fb036d6a86e..b2ce5c0807fb 100644
@@ -449,7 +449,7 @@ static ssize_t hdpvr_read(struct file *file, char __user *buffer, size_t count,
 
 		if (buf->status != BUFSTAT_READY &&
 		    dev->status != STATUS_DISCONNECTED) {
-			int err;
+
 			/* return nonblocking */
 			if (file->f_flags & O_NONBLOCK) {
 				if (!ret)

@@ -457,23 +457,19 @@ static ssize_t hdpvr_read(struct file *file, char
 __user *buffer, size_t count, goto err;
 			}
 
-			err =
 wait_event_interruptible_timeout(dev->wait_data,
+			ret =

wait_event_interruptible_timeout(dev->wait_data, buf->status == BUFSTAT_READY, msecs_to_jiffies(1000));

drivers/media/usb/hdpvr/hdpvr-video.c +2 -4
-			if (err < 0) {
-				ret = err;
+			if (ret < 0)
 				goto err;
-			}
-			if (!err) {
+			if (!ret) {
 				v4l2_dbg(MSG_INFO, hdpvr_debug,

&dev->v4l2_dev, "timeout: restart streaming\n");

drivers/media/usb/hdpvr/hdpvr-video.c +0 -1
 				hdpvr_stop_streaming(dev);
-				msecs_to_jiffies(4000);

drivers/media/usb/hdpvr/hdpvr-video.c

Quoted 10 lines
drivers/media/usb/hdpvr/hdpvr-video.c +3 -4
-				err = hdpvr_start_streaming(dev);
-				if (err) {
-					ret = err;
+				msleep(4000);
+				ret = hdpvr_start_streaming(dev);
+				if (ret)
 					goto err;
-				}
 			}
 		}

Thanks,
Mauro