microdvd parser should read fps only from first line with {1}{1} prefix

[milahu]

frame rate should be parsed from the first line
only when the first line has the format

\{1\}\{1\}(\d+(?:\.\d*)?)

examples

{1}{1}25.000
{1}{1}30
{1}{1}60.
{1}{1}123.456
{1}{1}5
{1}{1}1

failing tests

import pysubs2

text = """\
{10}{20}666
"""
subs = pysubs2.SSAFile.from_string(text, format_="microdvd")
print("fps:", subs.fps)
# this works, frame rate is now 666

text = """\
{10}{20}asdf
"""
subs = pysubs2.SSAFile.from_string(text, format_="microdvd")
# ValueError: could not convert string to float: 'asdf'
# pysubs2.exceptions.UnknownFPSError: Framerate was not specified and cannot be read from the MicroDVD file.

downstream issue smacke/ffsubsync#176

[tkarabela]

Hi @milahu, can you tell me what you wish the library did instead of raising UnknownFPSError in this case?

If you want to assume a "default fps", you can do this already:

try:
    subs = pysubs2.SSAFile.from_string(text, format_="microdvd")
except pysubs2.exceptions.UnknownFPSError:
    subs = pysubs2.SSAFile.from_string(text, format_="microdvd", fps=25)

However, unless both the input and output format is MicroDVD, this will mess up timing for any subtitles that are not the assumed default (eg. 25 fps) and do not have the declaration in the first subtitle.

[milahu]

frame rate should be parsed from the first line only when the first line has the format

\{1\}\{1\}(\d+(?:\.\d*)?)

examples

{1}{1}25.000
{1}{1}30
{1}{1}60.
{1}{1}123.456
{1}{1}5
{1}{1}1

[tkarabela]

The library internally works with milliseconds, not frames - specifying framerate is not optional when reading MicroDVD.

• One can specify the framerate directly as the fps parameter
  • (in this case, the library does not attempt to parse anything from the subtitle text)
• If the fps parameter is not given, the library tries to read the framerate value from text of the first subtitle
• If reading framerate from the first subtitle fails (eg. {10}{20}asdf), then the library bails out with UnknownFPSError

I'm not sure what more the library could be doing to figure out what framerate should be used. The only other option I see is to supply a default framerate in case the autodetection fails, which is already possible via the code I suggested earlier:

try:
    subs = pysubs2.SSAFile.from_string(text, format_="microdvd")
except pysubs2.exceptions.UnknownFPSError:
    subs = pysubs2.SSAFile.from_string(text, format_="microdvd", fps=25)

Maybe I'm misunderstanding the issue, feel free to tell me more about the use-case where this is a problem :)

[milahu]

the parser should be more strict

it should parse the FPS only from first lines like

{1}{1}25.000
{1}{1}30
{1}{1}60.
{1}{1}123.456
{1}{1}5
{1}{1}1

but NOT from first lines like

{10}{20}666

sorry, my issue title was misleading

[tkarabela]

Ah, maybe I'm beginning to understand, let's go back to your initial examples.

Example 1
Would you like to see this throw UnknownFPSError as well?

import pysubs2

text = """\
{10}{20}666
"""
subs = pysubs2.SSAFile.from_string(text, format_="microdvd")
# raises pysubs2.exceptions.UnknownFPSError: Framerate was not specified and cannot be read from the MicroDVD file.

Example 2
What should happen in this case? I believe this is already correct, it raises UnknownFPSError (ValueError is caught internally and not propagated to the caller)

text = """\
{10}{20}asdf
"""
subs = pysubs2.SSAFile.from_string(text, format_="microdvd")
# raises pysubs2.exceptions.UnknownFPSError: Framerate was not specified and cannot be read from the MicroDVD file.

[tkarabela]

BTW, this is the relevant code:

pysubs2/pysubs2/microdvd.py

Lines 22 to 46 in f83b818

	@classmethod
	def from_file(cls, subs, fp, format_, fps=None, **kwargs):
	"""See :meth:`pysubs2.formats.FormatBase.from_file()`"""
	for line in fp:
	match = MICRODVD_LINE.match(line)
	if not match:
	continue
	fstart, fend, text = match.groups()
	fstart, fend = map(int, (fstart, fend))
	if fps is None:
	# We don't know the framerate, but it is customary to include
	# it as text of the first subtitle. In that case, we skip
	# this auxiliary subtitle and proceed with reading.
	try:
	fps = float(text)
	subs.fps = fps
	continue
	except ValueError:
	raise UnknownFPSError("Framerate was not specified and "
	"cannot be read from "
	"the MicroDVD file.")
	start, end = map(partial(frames_to_ms, fps=fps), (fstart, fend))

[milahu]

Example 1
Would you like to see this throw UnknownFPSError as well?

yes

[moi15moi]

VLC also only parse the fps when it has this format: "{1}{1}THE_FPS"

Reference: https://code.videolan.org/videolan/vlc/-/blob/dccda0e133ff0a2e85de727cf19ddbc634f06b67/modules/demux/subtitle.c#L1014

[tkarabela]

Thanks, I see! I will implement this.

[milahu]

trivial...

--- a/pysubs2/microdvd.py
+++ b/pysubs2/microdvd.py
@@ -34,6 +34,12 @@ class MicroDVDFormat(FormatBase):
                 # We don't know the framerate, but it is customary to include
                 # it as text of the first subtitle. In that case, we skip
                 # this auxiliary subtitle and proceed with reading.
+                if fstart != 1 or fend != 1:
+                    # the first subtitle must have this format for fps=25:
+                    # {1}{1}25
+                    raise UnknownFPSError("Framerate was not specified and "
+                                          "cannot be read from "
+                                          "the MicroDVD file.")
                 try:
                     fps = float(text)
                     subs.fps = fps

[tkarabela]

Fixed in version 1.7.0.