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

[Jason Tackaberry]

Hi!

Thanks for your very comprehensive review.  Below are my responses to
your comments.  Where I haven't responded, it's because I agree with you
and will include your suggestions (and in some cases corrections):

On Sun, 2005-09-11 at 22:20 -0400, The Wanderer wrote:
> 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.

Yes, I'm so terrible at this.  If I don't reply within an hour or two, I
probably never will (unless nagged again).

> > +vf_osd allows an application controlling MPlayer in slave mode (hence
> > +called "the application") 
> 
> 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.

In fact, I had originally had "henceforth," which is certainly correct,
but as you say, it sounded so legalese. :)  I do believe "hence" is
technically correct here, though, as "from this time" is one meaning of
the word.  Still, I've no attachment to the word, and I could just as
easily say "from this point on called ..."

> 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.

Use "which" when the clause that follows is incidental and doesn't
fundamentally change the meaning of the sentence.  (And a comma always
precedes "which.")  Use "that" when the subsequent clause modifies or
limits a noun.  I think it's called a restrictive clause.  I remember
this was a pet peeve of one of my English profs in university.  He
ranted that people regularly use "which" because it sounds smarter, even
though it's technically wrong.

For the same reason, people sometimes use "whom" as a verb subject.
It's really funny when somebody misuses "whom" in speech.  He used to
say, "If you're going to say 'whom,' make damn sure you're using it
properly, or else you deserve what you get."

Anyway, "an OSD buffer that is not changing" is correct, because "is not
changing" is limiting "OSD" and is not incidental or optional.


> > +   * 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.

How about: "Fixed OSD frame rate (of about 30 fps), irrespective of the
video's frame rate or pause state"?

> > +a file is loaded whose destination (i.e. aspect-corrected) size is different
> 
> I'd probably prefer a comma after "i.e." in this case.

I think I could effectively argue it's a matter of style.  But speaking
of style, the Chicago Manual of Style recommends the comma, so I'll add
it. :) 

> > +vf_osd will create a System V shared memory buffer with the key 1234567 that
> 
> 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.

It is System V shared memory as opposed to POSIX shared memory.
Linguistically there's nothing wrong with your suggestion, but I would
argue in favor of my current form for two reasons: "System V shared
memory" is a common and understood phrase; "System V-style" looks
awkward because English has unclear order of operations and lacks
parentheses, and an ambiguity is created: does the "style" apply to "V"
or "System V"?  Capitalization helps to remove (but not eliminate the
ambiguity).  Of course, the target audience (which is not end-users but
developers) will have no problem realizing that the proper parsing is
(System V)-style.  But the target audience will also have no trouble
comprehending "System V shared memory" either, which is equally
grammatically correct.

You might call me pedantic.  But then so are you, so ... :)

> > +   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.

I suppose the term exposes the internal logic: there is a
synchronization between the BGRA (shared memory) OSD buffer and the
YV12A (internal) buffer.  This command marks the specified region of the
YV12 buffer as invalid and causes that region to be converted from BGRA
to YV12 on the next frame.

One might suggest "update" as a better command, but "update" implies an
immediacy, whereas it's more of a "queue on next frame draw" action.

The term is also borrowed from at least gdk, which provides a function
to invalidate a region of a window that indicates that region requires
updating.

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

It can't hurt to be explicit, I agree.  But is there any doubt
whatsoever that the units here are pixels? :)

> > +   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?

Correct.

> > +      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".

Good eye. :)

> > +.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.

Of course, MPlayer's man page is absolutely riddled with these.  But we
can at least prevent it from getting worse. :)

Thanks again for taking the time to proof my writing.

Cheers,
Jason.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: This is a digitally signed message part
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20050911/0ab19c97/attachment.pgp>