[MPlayer-dev-eng] [PATCH] vf_osd updates - fully baked?

[The Wanderer]

Jason Tackaberry wrote:

> Hi again,
> 
> Here is an updated patch of vf_osd with the following changes:

I kept meaning to comment on the docs in the previous patch, but somehow
never got that tuit. Rather than let it slip by me again, I'll avoid
procrastination and see what I think right now.

> +vf_osd allows an application controlling MPlayer in slave mode (hence
> +called "the application") to overlay image data on top of the video. It
> +operates both in features and in interface much like the OSD functionality
> +found in hardware such as the Hauppauge PVR-350. Here is a brief list of
> +features:

I'd probably say "henceforward", if not the legalistically correct
"hereinafter" which is probably overkill. There are also ways to
rephrase the sentence more extensively to avoid the current usage, which
is incorrect ("hence" used by itself in this context is a verb meaning
"therefore", which doesn't make sense here), but they may not be
necessary.

If it's accurate, I'd prefer a phrasing like "It bears a strong
similarity, both in features and in interface, to the OSD functionality
found in" et cetera. The advantage of this is in setting the "both in
features and in interface" sub-clause (I think that's the term) apart
better, for better sentence flow. If you don't want to change things
quite that much, then "Its operation, both in features and in interface,
is much like" would also work; I don't like it as well, because I don't
think "operation" is quite the right sort of term for this context, but
I wouldn't object too strongly.

> +   * Supports both per-pixel alpha and global alpha.
> +   * Compositing is MMX accelerated and performs quite well. On a 2GHz
> +     Sempron, it is possible to overlay a translucent video on the OSD
> +     with no frame loss in either video (and with 20% CPU to spare). For
> +     an OSD buffer that is not changing, vf_osd can composite an 800x600
> +     OSD at 24 fps with about a 7% CPU usage overhead (on the same 2GHz
> +     CPU).

A quick note: although the only statement I've been able to find about
when each is appropriate disagrees, I would probably prefer to say
"which is not changing" rather than "that is not changing". If you'd
rather not change it, I have no problems with that.

> +   * Fixed OSD frame rate (of about 30 fps) regardless of the video frame
> +     rate or whether the video is paused.

This doesn't seem quite ideal, but I'm not sure why not or how to fix
it; it's certainly tolerable, just not polished.

> +If the dimensions of the OSD are specified, the size of the OSD is fixed
> +between loadfiles. Otherwise, the OSD is called "unfixed" and is sized to
> +fit the video window. (Note that in this case, if the video has non-square
> +pixels and requires scaling, software scaling will be used implicitly.) If
> +a file is loaded whose destination (i.e. aspect-corrected) size is different
> +than the previous file and the OSD is unfixed, the previous shared memory
> +segment will be destroyed and a new one created.

I'd probably prefer a comma after "i.e." in this case.

I have a philosophical objection to the use of the construction
"different than", which I maintain despite lack of solid evidence to be
invalid. In this case, "different from that of" will work as a drop-in
replacement.

> +For example, MPlayer can be invoked this way:

Either "in this way" or "like this" (the former is more formal, the
latter more casual). For that matter, "like so" might also work.

Those are superficial 'fixes', for what may or may not be an underlying
problem. I'm not entirely sure what the problem (if any) is, but in
trying to track it down: is there a reason why you chose this particular
phrasing for the sentence?

(An alternative line might be "For example, consider this MPlayer
command line:".)

> +   mplayer -vf osd=1234567:800:600 movie.avi
> +
> +vf_osd will create a System V shared memory buffer with the key 1234567 that
> +holds an 800x600 BGRA image. The application will then access this buffer
> +using the given key. With the OSD dimensions fixed this way, it makes more
> +sense for the controlling application to explicitly use software scaling:

I would probably say "System V-style", if I'm correct in reading this to
mean that the type or style of shared memory used under System V is
different from that used in other situations and that the invocation
given will attempt to use the System V version.

(Beh. I'm not thinking as clearly as I'd like to be; it's very probable
that some of this will have to be looked at again later.)

> +    mplayer -vf scale=800:-2,expand=800:600,osd=1234567:800:600 movie.avi
> +
> +When the OSD filter is initialized, it will output a line such as:
> +this:

Extraneous colon after "as" - either that, or you need to drop the
"this:".

> +Internally, vf_osd maintains two buffers. The first buffer is the shared
> +BGRA buffer, which the application will render to. The second buffer is a
> +planar YUVA (YV12 plus alpha channel). The "YV12A" buffer is what is read
> +by vf_osd when compositing. Therefore an application writing to the shared
> +BGRA buffer isn't sufficient for the OSD to be updated. The application
> +must follow up with an "osdcmd invalidate" slave command to cause vf_osd to
> +synchronize its internal YV12A buffer with the BGRA buffer.

I'd insert a comma after "Therefore".

Perhaps "isn't sufficient to cause the OSD to be updated"?

> +NOTE: Currently the OSD update rate is only independent of the video frame
> +rate when double buffering is disabled (with the -nodouble switch). When
> +double buffering is enabled (which is the default behaviour) and the video
> +is playing, the OSD can only be updated as fast as the video frame rate.

I'd probably prefer something like "Currently, the only time the OSD
update rate is independent of the video frame rate is when double
buffering is disabled".

> +Multple commands "cmd" can be specified using only one "osdcmd" slave
> +command. Each "osdcmd" slave command is evaluated between video frames,
> +so if you want to perform multiple operations to the OSD before the next
> +frame is drawn (such as, for example, invalidating two different rectangles
> +and adjusting the global alpha) you must list those operations, separated
> +by commas, with one "osdcmd" slave command. Otherwise, if you had specified N
> +"osdcmd" commands, it would take N frames to update those changes.

"Multple" -> "Multiple"

I might say "Multiple OSD commands "cmd"", to further differentiate
between OSD commands and slave commands.

The phrasing "Each "osdcmd" slave command is evaluated between video
frames" does not imply "Only one "osdcmd" slave command is evaluated
between each pair of adjacent video frames", which is what you appear to
mean (given your later explication).

> +The values for "args" are dependent on "cmd". These are the supported
> +commands "cmd" for the "osdcmd" slave command.
> +
> +   invalidate=x:y:w:h
> +
> +      Cause the specified rectangle to be updated on the OSD. (Internally
> +      this forces BGRA->YV12A colorspace conversion.)

Why is this called "invalidate"? It doesn't seem to be connected with
what the command does; I don't see any way in which updating a rectangle
on the OSD is equivalent to causing anything to be non-valid.

Either add a comma after "Internally" or say something like "This forces
internal BGRA->YV12A colorspace conversion".

What units are x, y, w, and h in? This is relevant to most of the
options below, as well.

> +   slice=y:h
> +
> +      Clip the OSD viewport to the specified slice, where y is the top of
> +      the slice, and h is the height. This causes only the specified region
> +      of the OSD buffer to be composited onto the video. If there are only
> +      small regions of the OSD used, the application can use this as a hint
> +      to vf_osd to speed up rendering.

Drop the comma after the second "slice".

Am I correct in inferring that it is possible to, later, switch again to
another slice which is not a subset of the previous slice?

> +   visible=[0|1]
> +
> +      Draw OSD if 1, or don't draw OSD if 0.
> +
> +      vf_osd is initialized with visible=0, so the application will first
> +      have to set visible=1.

"so in order to display it, the application will"

> +Here are some examples:
> +
> +   osdcmd visible=1,invalidate=0:0:640:480
> +
> +      Sets the OSD visible, and causes the region of the OSD BGRA buffer
> +      starting at coordinates (0,0) (top left corner) with a width of
> +      640 pixels and height of 480 pixels to be updated.

Either drop the "a" before "width", or insert one before "height".

> --- main.orig/DOCS/man/en/mplayer.1	2005-09-11 12:47:18.000000000 -0400
> +++ main/DOCS/man/en/mplayer.1	2005-09-11 12:48:36.000000000 -0400

> +.IPs <width>
> +The width of the OSD buffer in pixels.
> +.IPs <height>
> +The height of the OSD buffer in pixels.

These are not full sentences (no verb), and so should not be
capitalized.

I've omitted commenting on the comments in the code itself, since that's
less critical from a put-up-a-good-front perspective and in any case
doesn't need as much polish; your writing is good enough for
comprehension in most cases, it's just that IMO documentation intended
for users should be as smooth as possible

-- 
       The Wanderer

Warning: Simply because I argue an issue does not mean I agree with any
side of it.

A government exists to serve its citizens, not to control them.