On Saturday 28 January 2006 22:11, David Abrahams wrote: > Isaac Richards <[EMAIL PROTECTED]> writes: > > On Saturday 28 January 2006 20:28, David Abrahams wrote: > >> It's pretty common, when I hit the 'E' key while watching a recording, > >> for the indicator to end up at zero. Likewise it's common for me to > >> end up back at the beginning of all video when I use the skip > >> forward/reverse keys. > > > > <snip> > > > >> As I said, this bug is apparently repeated through most of the > >> subclasses of VideoOutput, the exceptions being VideoOutputIvtv and > >> VideoOutputXv. I suppose most people are using Xv and that's why it > >> hasn't been more widely reported. Regardless, the repetition of this > >> code pattern strongly suggests that some refactoring is in order. > > > > No. VideoOutputXv has the same "bug", > > Well, it depends. When VideoOutputSubType() > XVideo it calls > PrepareFrameXvMC, where this->framesPlayed is never updated if buffer > turns out to be NULL. So, is that the way it's supposed to be?
Yes. XvMC pauses differently than Xv does. > > but that's how the code is supposed to > > be. VideoOutputIvtv doesn't even have analogous code. > > > > The problem is that VideoOutputQuartz::UpdatePauseFrame() isn't updating > > the scratch frame's frame number like it should, since it's using that > > style of pausing. > > Okay, so how should it update the scratch frame's frame number? And > why, if it's supposed to work that way, is it apparently perfect with > my change? What is broken that I'm not seeing? Check how it's done in VideoOutputXv::UpdatePauseFrame. Obviously, it works there. Simple grepping would have found that. > > This obviously is not a case for refactoring, since each output > > class can and does (XvMC and ivtv) need to handle this differently. > > It's not obvious at all. > > <soapbox> > Any time there's code repetition it's a case for refactoring. If > there's a "style of pausing" that's used by multiple VideoOutput > subclasses, and it requires updating the scratch frame's frame number > in some particular way, it should have been factored out. Had that > been done properly, this bug would never have appeared in the Mac > frontend without biting everyone else, too. So it would have been > noticed and fixed immediately. And of course, that's the > whole point of refactoring to eliminate code duplication. > </soapbox> No. We're talking about a single line of code duplication in a couple of child classes but not others, and which is dependant on the manner of operation in another of the sibling classes. Do you really think that a few hundred lines of email are necessary for a one line change for a simple bug? Seems rather silly to me. Make a tiny little patch, create a ticket in trac, attach it. Much easier. Isaac _______________________________________________ mythtv-dev mailing list mythtv-dev@mythtv.org http://mythtv.org/cgi-bin/mailman/listinfo/mythtv-dev
