Opened 5 years ago

Closed 4 years ago

#5401 closed defect (fixed)

Video processing fails when media contains no tags

Reported by: ayleph Owned by:
Priority: major Milestone:
Component: programming Keywords: video, processing, tags
Cc: Boris Bobrov Parent Tickets:

Description

The skip_transcode code in mediagoblin/media_types/video/util.py fails with an error if a file does not have metadata tags.

/path/to/mediagoblin/mediagoblin/media_types/video/transcoders.py:34: PyGIWarning: GstPbutils was imported without specifying a version first. Use gi.require_version('GstPbutils', '1.0') before import to ensure that the right version gets loaded.
  from gi.repository import GObject, Gst, GstPbutils
DEBUG:mediagoblin.processing.task:Processing <MediaEntry 7016: Cartel para adopción>
ERROR:mediagoblin.processing.task:An unhandled exception was raised while processing <MediaEntry 7016: Cartel para adopción>
WARNING:mediagoblin.processing:No idea what happened here, but it failed: AttributeError("'NoneType' object has no attribute 'get_string'",)
WARNING:mediagoblin.processing:No idea what happened here, but it failed: AttributeError("'NoneType' object has no attribute 'get_string'",)
Traceback (most recent call last):
  File "bin/gmg", line 9, in <module>
    load_entry_point('mediagoblin', 'console_scripts', 'gmg')()
  File "/path/to/mediagoblin/mediagoblin/gmg_commands/__init__.py", line 142, in main_cli
    args.func(args)
  File "/path/to/mediagoblin/mediagoblin/gmg_commands/reprocess.py", line 293, in reprocess
    run(args)
  File "/path/to/mediagoblin/mediagoblin/gmg_commands/reprocess.py", line 205, in run
    reprocess_info=reprocess_request)
  File "/path/to/mediagoblin/mediagoblin/submit/lib.py", line 261, in run_process_media
    task_id=entry.queued_task_id)
  File "/path/to/mediagoblin/lib/python2.7/site-packages/celery-3.1.17-py2.7.egg/celery/app/task.py", line 547, in apply_async
    link=link, link_error=link_error, **options)
  File "/path/to/mediagoblin/lib/python2.7/site-packages/celery-3.1.17-py2.7.egg/celery/app/task.py", line 739, in apply
    request=request, propagate=throw)
  File "/path/to/mediagoblin/lib/python2.7/site-packages/celery-3.1.17-py2.7.egg/celery/app/trace.py", line 355, in eager_trace_task
    uuid, args, kwargs, request)
  File "/path/to/mediagoblin/lib/python2.7/site-packages/celery-3.1.17-py2.7.egg/celery/app/trace.py", line 253, in trace_task
    I, R, state, retval = on_error(task_request, exc, uuid)
  File "/path/to/mediagoblin/lib/python2.7/site-packages/celery-3.1.17-py2.7.egg/celery/app/trace.py", line 240, in trace_task
    R = retval = fun(*args, **kwargs)
  File "/path/to/mediagoblin/mediagoblin/processing/task.py", line 101, in run
    processor.process(**reprocess_info)
  File "/path/to/mediagoblin/mediagoblin/media_types/video/processing.py", line 396, in process
    vp8_threads=vp8_threads, vorbis_quality=vorbis_quality)
  File "/path/to/mediagoblin/mediagoblin/media_types/video/processing.py", line 265, in transcode
    if skip_transcode(metadata, medium_size):
  File "/path/to/mediagoblin/mediagoblin/media_types/video/util.py", line 37, in skip_transcode
    if config['mime_types'] and tags.get_string('mimetype')[0]:
AttributeError: 'NoneType' object has no attribute 'get_string'

Here's a proposed fix. But I guess the question is, is it valid to have video with tag metadata? If not, then something else may be wrong (bad video file, or perhaps the wrong media type is trying to process the file).

diff --git a/mediagoblin/media_types/video/util.py b/mediagoblin/media_types/video/util.py
index d3d2927..e35b021 100644
--- a/mediagoblin/media_types/video/util.py
+++ b/mediagoblin/media_types/video/util.py
@@ -34,6 +34,9 @@ def skip_transcode(metadata, size):

     _log.debug('skip_transcode config: {0}'.format(config))
     tags = metadata.get_tags()
+    if not tags:
+        return False
+
     if config['mime_types'] and tags.get_string('mimetype')[0]:
         if not tags.get_string('mimetype')[1] in config['mime_types']:
             return False

Subtickets

Attachments (1)

0001-Check-all-tags-for-existence-before-using-them.patch (2.3 KB) - added by Boris Bobrov 5 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 5 years ago by ayleph

Encountered another similar issue in the same skip_transcode function where the code tried to compare a string against an empty video tag. Looks like we need to add checks for none in a lot of the code.

--- a/mediagoblin/media_types/video/util.py
+++ b/mediagoblin/media_types/video/util.py
@@ -48,7 +48,10 @@ def skip_transcode(metadata, size):

     if config['video_codecs']:
         for video_info in metadata.get_video_streams():
-            if not (video_info.get_tags().get_string('video-codec')[1] in
+            tags = video_info.get_tags()
+            if not tags:
+                return False
+            if not (tags.get_string('video-codec')[1] in
                     config['video_codecs']):
                 return False

comment:2 Changed 5 years ago by Boris Bobrov

Resolution: fixed
Status: newclosed

I've changed your patch and applied it as fec916d. Thank you for the patch!

comment:3 Changed 5 years ago by ayleph

Resolution: fixed
Status: closedaccepted

Re-opening because there were other issues the patch didn't address as mentioned in comment 1. We pretty much to riddle checks throughout the skip_transcode function.

comment:4 Changed 5 years ago by Boris Bobrov

oooh, right, my bad. Will re-fix.

comment:5 Changed 5 years ago by Boris Bobrov

Owner: set to Boris Bobrov
Status: acceptedin_progress

comment:6 Changed 5 years ago by Boris Bobrov

Owner: Boris Bobrov deleted
Status: in_progressreview

ayleph, what do you thing about the patch? Does it fix the problem for you?

comment:7 in reply to:  6 Changed 5 years ago by ayleph

Replying to breton:

ayleph, what do you thing about the patch? Does it fix the problem for you?

Definitely addresses the problem I was seeing with video_tags being empty on a real video file. It looks like that takes care of the null cases. I can't think of a good way to test whether the dimensions_match code could fail. So I'd say it looks good to push. Thanks!

comment:8 Changed 4 years ago by Boris Bobrov

Resolution: fixed
Status: reviewclosed

Merged in 339cfb3

Note: See TracTickets for help on using tickets.