[mythtv] DVB alpha-0.4

[Ben Bucksch]

Kenneth Aafloy wrote:

>anyone have anything to add?
>
Comparing to the last version I edited:

    * Thanks for picking up (most of) my changes.
    * Reading your changelog, thanks for these changes (warnings,
      removal of more pointers, use_transport_stream fixes etc.)
    * dvbchannel
          o You removed the Close() function. Makes no sense to me. It's
            small, but at least symmetric to Open() and dvbrecorder.
            Please keep it.
          o In my patch, I used |const QString&| instead of |QString|.
            If you're looking for speed, you should use that. I know you
            it breaks something, but I doubt that this specific change
            could have breaked anything.
          o Why are Tune(), PrintChannelOptions() and CheckChannel() now
            taking an argument? They are class methods, are invoked only
            once each, and the parameter used is a class property. So,
            the argument is useless, please remove it again, unless you
            have a specific reason for it.
          o     while (true)
            I missed that earlier. How long are you blocking here max,
            in ms? Isn't that a problem for other tasks in that thread?
    * dvbrecorder
          o Again, 3rd time: If you copy code, please add a comment
            *right* *above* the relevant code, where you got the code
            from, with author and code URL, in the way I did it in the
            last version I sent you. A comment in the license header at
            the top of the file doesn't tell *exactly* which part (up to
            line granularity) was copied, and it matters for code sync
            in case of bugs. Also, somebody looking at the code won't
            notice the comment at the top. It's doesn't hurt, but is
            actually important right there IMHO.
          o                  if (pid_ipack.find(pid) == pid_ipack.end())
                                     // PID not found in map, i.e. got
            unwanted PID
            Please keep that comment I added, the code is not obvious to
            people not familiar to |map|.
          o     pid_string = "Setting pids: Audio(";
                OpenFilters(pids.audio,       DMX_PES_AUDIO);
                pid_string += " ) Video(";
                OpenFilters(pids.video,       DMX_PES_VIDEO);
                pid_string += " )";
                GEN(pid_string);
            (3 times, plus more lines in another function)
            (with pid_string being a class property)
            This is messy and only saves a few linebreaks in the debug
            output. Just do
            -        pid_string += QString(" %1").arg(this_pid);
            +        GEN(QString(" %1 (type %2)").arg(this_pid).arg(type));
            If the cleartext type is important, add that there.
          o You remove #ifdef USING_DVB, and I don't see changes to the
            Makefile which would make that up. Does that still compile
            without DVB driver headers?
          o You removed
                if (cam)
                    cam->Stop();
                if (sections)
                    sections->Stop();
            Was that accidental or intentional?

Some less important or unimportant comments:

    * dvbchannel
          o Do you clear the |tuning| property somewhere during channel
            changes or may old values from the last channel prevail?
            That doesn't confuse the driver? Or is that even a good
            thing (garanteed to work and less tuning work needed)?
          o Correspondingly:
                    prev_tuning.params.frequency = tuning.params.frequency;
                    prev_tuning.params.inversion = tuning.params.inversion;
            The |prev_tuning = tuning;| at the end of the function
            didn't work?
          o Before checkin, it would be nice, if you could clean up the
            linebreaks, so that no line is longer than 80 chars. E.g.
            -        prev_tuning.params.u.ofdm.code_rate_HP !=
            tuning.params.u.ofdm.code_rate_HP ||
            +        prev_tuning.params.u.ofdm.code_rate_HP !=
            +                                         
            tuning.params.u.ofdm.code_rate_HP ||
    * dvbrecorder
          o You replaced
                            cerr << "Error reading from DVB device "
                                 << devicenodename(dvbdev_dvr,cardnum)
            << endl;

                            if (dvr.readsz == -1)
                                perror("ERROR");
                            else
                                cout << "error code :" << dvr.readsz <<
            endl;
            with
                            ERR("Error reading from DVB device.");
            Was the error msg useless?

BTW: Once the code is in CVS, please don't continue to re-arrange stuff 
without urgent need. Makes diffs totally useless and thus reviews very 
time-intensive.