10
\$\begingroup\$

The goal of this software is to read the proprietary file system index of a digital video recorder and to carve all of the video chunks into files within a dated directory structure.

I'm not very experienced and working in isolation so would like some feedback on the code in general in terms of style/efficiency and whatever else you may deem relevant.

import datetime
import os

from bitstring import ConstBitStream


class HikvisionDrive:

    def __init__(self, file_loc):
        # create data stream of hard drive
        self.file = file_loc
        self.data = ConstBitStream(filename=self.file)

        # get information from master sector
        self.data.pos = 584 * 8
        self.capacity = self.data.read('uintle:64')

        self.data.pos += 56 * 8
        self.data_block_size = self.data.read('uintle:64')
        self.total_data_blocks = self.data.read('uintle:32')

        self.data.pos += 4 * 8
        self.hikbtree_offset = self.data.read('uintle:64')
        self.hikbtree_size = self.data.read('uintle:32')

        # get information from hikbtree
        self.data.pos = (self.hikbtree_offset + 80) * 8
        self.page_list_offset = self.data.read('uintle:64')

        # initialise list for data blocks
        self.data_blocks = []

    def show_info(self):
        print('-Hard Drive Information-')
        print('File: ' + str(self.file))
        print('Capacity: ' + str(self.capacity) + ' bytes')
        print('Data Block Size: ' + str(self.data_block_size) + ' bytes')
        print('Number of Data Blocks: ' + str(self.total_data_blocks))
        print('HikBTree Offset: ' + str(self.hikbtree_offset))
        print('HikBTree Size: ' + str(self.hikbtree_size) + ' bytes')
        print('Page List Offset: ' + str(self.page_list_offset))

    def save_blocks(self, save_directory):
        # loop through data blocks and save the data into a structured directory
        # setup progress counter
        x = 1
        with open(self.file, 'rb') as f1:
            for data_block in self.data_blocks:
                # update progress
                print('saving data block #', x, 'of', self.total_data_blocks, '...')
                # set directory
                date = data_block['start_time']
                save_dir = save_directory + '\\' + str(date.year) + '-' + date.strftime('%B') + '\\'\
                    + date.strftime('%d')
                if not os.path.exists(save_dir):
                    os.makedirs(save_dir)
                # save the data to a file
                file_name = 'CH' + str(data_block['channel']) + '_' + date.strftime('%H') + '-' + date.strftime(
                    '%M') + '-' + date.strftime('%S') + '_block' + str(x) + '.raw'
                length = self.data_block_size
                f1.seek(data_block['offset'])
                with open(str(save_dir + '\\' + file_name), 'wb') as f2:
                    while length:
                        chunk = min(1024 * 1024, length)
                        data = f1.read(chunk)
                        f2.write(data)
                        length -= chunk
                f2.close()
                x += 1
            f1.close()

    def scan_pages(self):
        # create list of page offsets
        page_offsets = []
        # get the offset to Page #1
        self.data.pos = (self.page_list_offset + 24) * 8
        page_offsets.append(self.data.read('uintle:64'))
        # loop through the rest of the list
        self.data.pos += 64 * 8
        page_offset = self.data.read('uintle:64')
        while page_offset != 0:
            page_offsets.append(page_offset)
            self.data.pos += 40 * 8
            page_offset = self.data.read('uintle:64')
        # create list of data blocks from the pages
        for page in page_offsets:
            self.data.pos = (page + 96) * 8
            entry = True if self.data.read('hex:64') == 'ffffffffffffffff' else False
            while entry:
                video_exists = True if self.data.read('hex:64') == '0000000000000000' else False
                self.data.pos += 1 * 8
                channel = self.data.read('uint:8')
                self.data.pos += 6 * 8
                start_time = datetime.datetime.fromtimestamp(self.data.read('uintle:32'))
                end_time = datetime.datetime.fromtimestamp(self.data.read('uintle:32'))
                data_offset = self.data.read('uintle:64')
                self.data_blocks.append({'channel': channel, 'has_video': video_exists, 'start_time': start_time,
                                         'end_time': end_time, 'offset': data_offset})
                # move to next block entry
                self.data.pos += 8 * 8
                entry = True if self.data.read('hex:64') == 'ffffffffffffffff' else False


drive = HikvisionDrive(r'D:\213_HDD.001')
drive.show_info()
drive.scan_pages()
drive.save_blocks(r'D:\Carved Files')
\$\endgroup\$
7
\$\begingroup\$

There are a couple of obvious issues:

  1. Get used to writing docstrings. When you declare a function and then write a comment describing what it does, you're wrong. Python uses docstrings for this, which have the advantage that they can be accessed and manipulated at runtime (for example, in the REPL command line):

    def save_blocks(self, save_directory):
        # loop through data blocks and save the data into a structured directory
        # setup progress counter
    

    No! Do this instead:

    def save_blocks(self, save_directory):
        """Loop through data blocks and save the data into a structured directory.
    
        """
        # setup progress counter
    
  2. You're not looping like a native. See this link to Ned Batchelder's "Loop like a Native" talk for slides and video.

    In particular, you are writing "read and store" code, when you should be writing "generate when needed" code. There's no reason to process the blocks by first loading all of them into memory. And, considering you are talking about video files, there's plenty of reasons not to load all of them into memory! Instead, use a generator to yield one item at a time, as needed.

    And that whole x += 1 thing. Ugh! enumerate!

  3. Use the library! Say "Hello!" to my leetle friend: pathlib

    date = data_block['start_time']
    save_dir = save_directory + '\\' + str(date.year) + '-' + date.strftime('%B') + '\\'\
        + date.strftime('%d')
    if not os.path.exists(save_dir):
        os.makedirs(save_dir)
    

    Becomes:

    from pathlib import Path    # At Top of module
    
    
    date = data_block['start_time']
    save_path = Path(save_directory) / date.strftime('%Y-%B') / date.strftime('%d')
    
    if not save_path.exists():
        save_path.mkdir(parents=True)
    

    Note the changes to strftime, but I kept the subdirectory %d separate to allow Path's operator-/ to do its magic on Windows/Unix paths.

\$\endgroup\$
  • \$\begingroup\$ Thanks! I am with you on the docstrings, enumeration and pathlib library. All very helpful tips. I'm not sure I have understood the generate when needed though, is this referring to me saving the data_block information to a list and then having another method to carve the data_blocks? \$\endgroup\$ – DanWheeler Dec 15 '17 at 23:05
  • 2
    \$\begingroup\$ Pretty much any time you find yourself writing = [] you should ask, "Am I about to store a list that I should just generate on the fly?" \$\endgroup\$ – Austin Hastings Dec 15 '17 at 23:12
  • \$\begingroup\$ Ok, I've also just got to the generator part of the video you linked so hopefully i'll understand shortly haha. Thanks for your help. \$\endgroup\$ – DanWheeler Dec 15 '17 at 23:14
4
\$\begingroup\$

Okay I know I'm late to the party but I wanted to give my impressions on this code too.

  • Functions/methods are too long and are doing too much. When a function is longer than a screenful, it becomes very hard to keep everything in mind. Functions should obey the single responsibility principle: have one clearly defined task that it performs.
  • Too many magic numbers. There are numbers and strings scattered throughout the code and I have no idea where any of them come from or what they mean. Some of them have comments near them, but it's still inscrutable.
  • Lots of repeated code. For instance all the self.data.read('uintle:64') Maybe should be their own method.
  • Split out the generation of the string in show_info() into a new method __str__() since representing making the object readable. Then show_info() should just call str(self)
  • Need some error/exception detection in places where you work with files.
  • Don't call print() in the main logic of your code. What if you code has to be run some place where there isn't a terminal, such as a batch process or GUI? Instead look into using the logging module.
\$\endgroup\$
  • \$\begingroup\$ Thanks for the tips! With regards self.data.read('uintle:64') being its own method. Is it not already its own method just as part of another class? I suppose i could have a specific method called "read_64_bits" that does not require an argument but I'm not sure i understand the benefit. Is it readability? \$\endgroup\$ – DanWheeler Dec 19 '17 at 7:47
  • 1
    \$\begingroup\$ @DanWheeler Readability is a concern, yes. Maintainability is the other, more important one. It's because "uintle:64" is a "magic" string, and it's easy to get wrong. When I was trying to answer I typed "unitle64" the first time, even though I thought I was being careful. If the data format ever changes and you need another 64-bit unsigned int, you have to make sure you get the string right. Either the string itself should be put into a variable, or the whole thing should be its own method. Don't repeat yourself \$\endgroup\$ – Snowbody Dec 19 '17 at 14:12
  • \$\begingroup\$ Of course, that makes perfect sense. And for what it's worth, placing it into a method has made the code much nicer to read. Along with clearing out the rest of the magic numbers and trimming the functions down. \$\endgroup\$ – DanWheeler Dec 19 '17 at 14:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.