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

[The Wanderer]

Jason Tackaberry wrote:

> 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):

The parts you didn't respond to included some "this choice, or that
choice" sections (in which you may of course choose either silently
without problems), some "this choice, or something you pick yourself"
(in which it might be a good idea to run your own choice past me to see
what I think), and at least one outright question which didn't get
answered. Was this intentional?

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

I'm not quite that bad about it, but I've got a combined to-address
backlog of MPlayer-related messages (mainly patches and commits) of...
39 messages, plus possibly as many as 5 over in FFmpeg. Many of them are
months old, some possibly closer to a year.

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

I'm still not entirely satisfied with that, but it does work.

And yes, you're right about "hence" not being a verb, but I wasn't
entirely awake at the time of my previous post. (Well... awake, yes, but
my mind was still fairly blurry.)

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

The latter was the gist of the statement I'd found: if there was a comma
beforehand, use "which", otherwise use "that". It clashes with my sense
of what is and is not "right", but...

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

I tend not to use "whom" much, probably because I don't get much
occasion where it would be appropriate, but I'm something like 80%
confident that anywhere I do use it is correct. ^_^

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

Slightly more technical, which is why I didn't want to suggest a
phrasing like that, but it does fix the problem.

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

I recommend the comma based (when I bother to think about "why"s long
enough to dig it up) on the fact that if you carry out the
meaning-equivalent replacement of "i.e." with "that is", the comma is
obviously necessary. In practice I don't bother doing the mental
replacement to check, I just recognize that it seems off, but that's the
underlying justification insofar as I can figure it out.

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

Right. I was not specifically aware of the terms, so I didn't know what
the alternative was (or "s were", if there was more than one).

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

In point of fact, I've objected to many hyphenation usages in the docs
in the past on these exact grounds; this is in part the reason for the
fact that the docs tend not to use hyphens at all in most places. (That
wasn't my preferred solution - in fact I'm not fond of it - but it is
the one which got adopted.)

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

My primary objection, I think, was on such grounds as the fact that it
was not clear what the alternative types of shared memory were and/or
when each would be appropriate - since I'd never heard of multiple types
to begin with, and wasn't actively aware that System V tropes were still
relevant to most modern users.

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

But of course. ^_^

Objection withdrawn.

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

I'd have no problem with the name if this were explained, however
briefly, in the documentation somewhere. I do think that commands should
be at least vaguely intuitive, not necessarily in the sense that someone
might be able to guess them without instructions but in the sense that
they should make sense once known.

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

I might disagree about that implication, but I don't think I'm going to
push on this too strongly.

(In any case, is there anything which can be done between the issuing of
this command and the drawing of the next frame? If not, then the
immediacy might be appropriate anyway.)

(Hmm. If the 'invalidate' is not the last command in the osdcmd line,
then yes, other things will happen before the drawing of the next
frame... so the preceding paragraph is probably itself invalid.)

>> 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? :)

There could have been, yes, but I'll agree that pixels are the most
sensical unit to use.

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

This means that y and h (which, incidentally, I presume have those
labels here because it aligns with the way the labels were used in the
preceding command?) are counted starting from the top of the entire
image, not from the top of the current section. As may be inferred from
the fact that I felt the need to ask, this was not obvious to me; it
might be worth making it explicit, although brief experimentation would
suffice to answer the question for anyone with enough technical
sophistication to use the filter in the first place.

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

It is? I thought Diego had caught most of them already... if you want to
submit a patch to correct any of the ones which are still present, I'm
sure it would get committed.

> Thanks again for taking the time to proof my writing.

Just doin' my part...

(I ignore - or rather, postpone - 'way too many docs-related patches as
it is; I'd put yours off twice already, in the two previous versions,
and felt I should probably get to it *now* so as not to have it
committed before I had a chance to see what needed addressing.)

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