Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dev v2 #67

Closed
wants to merge 9 commits into from
212 changes: 106 additions & 106 deletions app/controllers/v1/video.py
Original file line number Diff line number Diff line change
Expand Up @@ -163,109 +163,109 @@ def delete_video(request: Request, task_id: str = Path(..., description="Task ID
)


@router.get(
"/musics", response_model=BgmRetrieveResponse, summary="Retrieve local BGM files"
)
def get_bgm_list(request: Request):
suffix = "*.mp3"
song_dir = utils.song_dir()
files = glob.glob(os.path.join(song_dir, suffix))
bgm_list = []
for file in files:
bgm_list.append(
{
"name": os.path.basename(file),
"size": os.path.getsize(file),
"file": file,
}
)
response = {"files": bgm_list}
return utils.get_response(200, response)


@router.post(
"/musics",
response_model=BgmUploadResponse,
summary="Upload the BGM file to the songs directory",
)
def upload_bgm_file(request: Request, file: UploadFile = File(...)):
request_id = base.get_task_id(request)
# check file ext
if file.filename.endswith("mp3"):
song_dir = utils.song_dir()
save_path = os.path.join(song_dir, file.filename)
# save file
with open(save_path, "wb+") as buffer:
# If the file already exists, it will be overwritten
file.file.seek(0)
buffer.write(file.file.read())
response = {"file": save_path}
return utils.get_response(200, response)

raise HttpException(
"", status_code=400, message=f"{request_id}: Only *.mp3 files can be uploaded"
)


@router.get("/stream/{file_path:path}")
async def stream_video(request: Request, file_path: str):
tasks_dir = utils.task_dir()
video_path = os.path.join(tasks_dir, file_path)
range_header = request.headers.get("Range")
video_size = os.path.getsize(video_path)
start, end = 0, video_size - 1

length = video_size
if range_header:
range_ = range_header.split("bytes=")[1]
start, end = [int(part) if part else None for part in range_.split("-")]
if start is None:
start = video_size - end
end = video_size - 1
if end is None:
end = video_size - 1
length = end - start + 1

def file_iterator(file_path, offset=0, bytes_to_read=None):
with open(file_path, "rb") as f:
f.seek(offset, os.SEEK_SET)
remaining = bytes_to_read or video_size
while remaining > 0:
bytes_to_read = min(4096, remaining)
data = f.read(bytes_to_read)
if not data:
break
remaining -= len(data)
yield data

response = StreamingResponse(
file_iterator(video_path, start, length), media_type="video/mp4"
)
response.headers["Content-Range"] = f"bytes {start}-{end}/{video_size}"
response.headers["Accept-Ranges"] = "bytes"
response.headers["Content-Length"] = str(length)
response.status_code = 206 # Partial Content

return response


@router.get("/download/{file_path:path}")
async def download_video(_: Request, file_path: str):
"""
download video
:param _: Request request
:param file_path: video file path, eg: /cd1727ed-3473-42a2-a7da-4faafafec72b/final-1.mp4
:return: video file
"""
tasks_dir = utils.task_dir()
video_path = os.path.join(tasks_dir, file_path)
file_path = pathlib.Path(video_path)
filename = file_path.stem
extension = file_path.suffix
headers = {"Content-Disposition": f"attachment; filename={filename}{extension}"}
return FileResponse(
path=video_path,
headers=headers,
filename=f"{filename}{extension}",
media_type=f"video/{extension[1:]}",
)
# @router.get(
# "/musics", response_model=BgmRetrieveResponse, summary="Retrieve local BGM files"
# )
# def get_bgm_list(request: Request):
# suffix = "*.mp3"
# song_dir = utils.song_dir()
# files = glob.glob(os.path.join(song_dir, suffix))
# bgm_list = []
# for file in files:
# bgm_list.append(
# {
# "name": os.path.basename(file),
# "size": os.path.getsize(file),
# "file": file,
# }
# )
# response = {"files": bgm_list}
# return utils.get_response(200, response)
#

# @router.post(
# "/musics",
# response_model=BgmUploadResponse,
# summary="Upload the BGM file to the songs directory",
# )
# def upload_bgm_file(request: Request, file: UploadFile = File(...)):
# request_id = base.get_task_id(request)
# # check file ext
# if file.filename.endswith("mp3"):
# song_dir = utils.song_dir()
# save_path = os.path.join(song_dir, file.filename)
# # save file
# with open(save_path, "wb+") as buffer:
# # If the file already exists, it will be overwritten
# file.file.seek(0)
# buffer.write(file.file.read())
# response = {"file": save_path}
# return utils.get_response(200, response)
#
# raise HttpException(
# "", status_code=400, message=f"{request_id}: Only *.mp3 files can be uploaded"
# )
#
#
# @router.get("/stream/{file_path:path}")
# async def stream_video(request: Request, file_path: str):
# tasks_dir = utils.task_dir()
# video_path = os.path.join(tasks_dir, file_path)
# range_header = request.headers.get("Range")
# video_size = os.path.getsize(video_path)
# start, end = 0, video_size - 1
#
# length = video_size
# if range_header:
# range_ = range_header.split("bytes=")[1]
# start, end = [int(part) if part else None for part in range_.split("-")]
# if start is None:
# start = video_size - end
# end = video_size - 1
# if end is None:
# end = video_size - 1
# length = end - start + 1
#
# def file_iterator(file_path, offset=0, bytes_to_read=None):
# with open(file_path, "rb") as f:
# f.seek(offset, os.SEEK_SET)
# remaining = bytes_to_read or video_size
# while remaining > 0:
# bytes_to_read = min(4096, remaining)
# data = f.read(bytes_to_read)
# if not data:
# break
# remaining -= len(data)
# yield data
#
# response = StreamingResponse(
# file_iterator(video_path, start, length), media_type="video/mp4"
# )
# response.headers["Content-Range"] = f"bytes {start}-{end}/{video_size}"
# response.headers["Accept-Ranges"] = "bytes"
# response.headers["Content-Length"] = str(length)
# response.status_code = 206 # Partial Content
#
# return response
#
#
# @router.get("/download/{file_path:path}")
# async def download_video(_: Request, file_path: str):
# """
# download video
# :param _: Request request
# :param file_path: video file path, eg: /cd1727ed-3473-42a2-a7da-4faafafec72b/final-1.mp4
# :return: video file
# """
# tasks_dir = utils.task_dir()
# video_path = os.path.join(tasks_dir, file_path)
# file_path = pathlib.Path(video_path)
# filename = file_path.stem
# extension = file_path.suffix
# headers = {"Content-Disposition": f"attachment; filename={filename}{extension}"}
# return FileResponse(
# path=video_path,
# headers=headers,
# filename=f"{filename}{extension}",
# media_type=f"video/{extension[1:]}",
# )
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

该代码看起来是一个 FastAPI 应用,提供了几个 API 端点用于管理视频和音乐文件。以下是一些代码审查的结果:

注释和文档
代码中缺乏注释和文档,这对于代码的维护和理解是很有帮助的。建议添加注释和文档来解释代码的目的和功能。

错误处理
代码中有一些错误处理,但是它并不是很完善。例如,在 upload_bgm_file 函数中,如果文件上传失败,会抛出一个 HttpException,但是这个错误信息并不详细。建议增加详细的错误信息和错误处理机制。

文件路径处理
代码中使用了 os.path.join 来处理文件路径,这是正确的。但是,建议使用 pathlib 库来处理文件路径,因为它更方便和安全。

文件类型检查
upload_bgm_file 函数中,只检查了文件扩展名是否为 .mp3,但是并没有检查文件的类型是否是真正的音频文件。建议增加文件类型检查来确保文件是真正的音频文件。

视频流和下载
视频流和下载功能实现了,但是建议优化这些功能来提高性能和安全性。例如,可以使用 StreamingResponse 来实现视频流,而不需要将整个视频文件加载到内存中。

文件上传
文件上传功能实现了,但是建议增加文件上传的大小限制和文件类型限制来防止恶意用户上传很大的文件或非允许的文件类型。

路由
路由设计得比较混乱,建议重新设计路由来提高代码的可读性和维护性。例如,可以使用子路由来分组相关的 API 端点。

测试
代码中缺乏测试,这对于代码的可靠性和维护性是很重要的。建议增加测试来确保代码的正确性和稳定性。

其他
代码中有一些重复的代码,建议提取出重复的代码来提高代码的可读性和维护性。

11 changes: 11 additions & 0 deletions app/controllers/v2/base.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
from fastapi import APIRouter, Depends


def v2_router(dependencies=None):
router = APIRouter()
router.tags = ["V2"]
router.prefix = "/api/v2"
# 将认证依赖项应用于所有路由
if dependencies:
router.dependencies = dependencies
return router
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这段代码定义了一个函数 v2_router,用于创建一个 FastAPI 路由器(router)。以下是一些代码审查意见和建议:

bug 风险:

  • dependencies 参数类型没有被限制,理论上可以传入任何类型的数据。如果传入的数据不符合 FastAPI 的期望类型,可能会导致异常。建议增加类型提示,例如 dependencies: List[Depends] = None
  • 如果传入的 dependencies 参数中包含相同的认证依赖项,可能会导致重复注册的错误。
  • router.tagsrouter.prefix 的赋值没有做任何错误处理,如果赋值失败(例如,tags 不是列表类型),可能会导致未捕获的异常。

改进建议:

  • 考虑增加文档注释,描述 v2_router 函数的作用和参数含义。
  • 考虑增加类型提示,明确 dependencies 参数的类型和范围。
  • 考虑增加错误处理,捕获可能的异常并返回合理的错误信息。
  • 考虑增加测试用例,确保 v2_router 函数的正确性和robust性。

改进后的代码示例:

from fastapi import APIRouter, Depends
from typing import List, Optional

def v2_router(dependencies: Optional[List[Depends]] = None) -> APIRouter:
    """
    创建一个 FastAPI 路由器,应用于 V2 API。
    
    Args:
    dependencies (List[Depends], optional): 认证依赖项列表。 Defaults to None.
    
    Returns:
    APIRouter: FastAPI 路由器实例。
    """
    try:
        router = APIRouter()
        router.tags = ["V2"]
        router.prefix = "/api/v2"
        
        if dependencies:
            router.dependencies = dependencies
        
        return router
    except Exception as e:
        # 添加错误处理和日志记录
        print(f"Error creating V2 router: {e}")
        raise

170 changes: 170 additions & 0 deletions app/controllers/v2/script.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,170 @@
from fastapi import APIRouter, BackgroundTasks
from loguru import logger
import os

from app.models.schema_v2 import (
GenerateScriptRequest,
GenerateScriptResponse,
CropVideoRequest,
CropVideoResponse,
DownloadVideoRequest,
DownloadVideoResponse,
StartSubclipRequest,
StartSubclipResponse
)
from app.models.schema import VideoClipParams
from app.services.script_service import ScriptGenerator
from app.services.video_service import VideoService
from app.utils import utils
from app.controllers.v2.base import v2_router
from app.models.schema import VideoClipParams
from app.services.youtube_service import YoutubeService
from app.services import task as task_service

router = v2_router()


@router.post(
"/scripts/generate",
response_model=GenerateScriptResponse,
summary="同步请求;生成视频脚本 (V2)"
)
async def generate_script(
request: GenerateScriptRequest,
background_tasks: BackgroundTasks
):
"""
生成视频脚本的V2版本API
"""
task_id = utils.get_uuid()

try:
generator = ScriptGenerator()
script = await generator.generate_script(
video_path=request.video_path,
video_theme=request.video_theme,
custom_prompt=request.custom_prompt,
skip_seconds=request.skip_seconds,
threshold=request.threshold,
vision_batch_size=request.vision_batch_size,
vision_llm_provider=request.vision_llm_provider
)

return {
"task_id": task_id,
"script": script
}

except Exception as e:
logger.exception(f"Generate script failed: {str(e)}")
raise


@router.post(
"/scripts/crop",
response_model=CropVideoResponse,
summary="同步请求;裁剪视频 (V2)"
)
async def crop_video(
request: CropVideoRequest,
background_tasks: BackgroundTasks
):
"""
根据脚本裁剪视频的V2版本API
"""
try:
# 调用视频裁剪服务
video_service = VideoService()
task_id, subclip_videos = await video_service.crop_video(
video_path=request.video_origin_path,
video_script=request.video_script
)
logger.debug(f"裁剪视频成功,视频片段路径: {subclip_videos}")
logger.debug(type(subclip_videos))
return {
"task_id": task_id,
"subclip_videos": subclip_videos
}

except Exception as e:
logger.exception(f"Crop video failed: {str(e)}")
raise


@router.post(
"/youtube/download",
response_model=DownloadVideoResponse,
summary="同步请求;下载YouTube视频 (V2)"
)
async def download_youtube_video(
request: DownloadVideoRequest,
background_tasks: BackgroundTasks
):
"""
下载指定分辨率的YouTube视频
"""
try:
youtube_service = YoutubeService()
task_id, output_path, filename = await youtube_service.download_video(
url=request.url,
resolution=request.resolution,
output_format=request.output_format,
rename=request.rename
)

return {
"task_id": task_id,
"output_path": output_path,
"resolution": request.resolution,
"format": request.output_format,
"filename": filename
}

except Exception as e:
logger.exception(f"Download YouTube video failed: {str(e)}")
raise


@router.post(
"/scripts/start-subclip",
response_model=StartSubclipResponse,
summary="异步请求;开始视频剪辑任务 (V2)"
)
async def start_subclip(
request: VideoClipParams,
task_id: str,
subclip_videos: dict,
background_tasks: BackgroundTasks
):
"""
开始视频剪辑任务的V2版本API
"""
try:
# 构建参数对象
params = VideoClipParams(
video_origin_path=request.video_origin_path,
video_clip_json_path=request.video_clip_json_path,
voice_name=request.voice_name,
voice_rate=request.voice_rate,
voice_pitch=request.voice_pitch,
subtitle_enabled=request.subtitle_enabled,
video_aspect=request.video_aspect,
n_threads=request.n_threads
)

# 在后台任务中执行视频剪辑
background_tasks.add_task(
task_service.start_subclip,
task_id=task_id,
params=params,
subclip_path_videos=subclip_videos
)

return {
"task_id": task_id,
"state": "PROCESSING" # 初始状态
}

except Exception as e:
logger.exception(f"Start subclip task failed: {str(e)}")
raise
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这是一个使用FastAPI框架的API代码。以下是一些代码评审的建议和发现的潜在问题:

  1. 异常处理: 代码中使用了try-except块来捕获异常,并使用logger记录异常信息。但是,并没有对异常进行分类处理,只是简单地记录并重新抛出异常。建议根据异常类型进行不同的处理和返回结果。
  2. 参数验证: 代码中没有对输入参数进行验证和处理。建议使用Pydantic的参数验证和默认值功能来确保输入参数的正确性和完整性。
  3. 异步任务: 代码中使用了BackgroundTasks来创建异步任务。建议使用更高级的异步任务库,例如 Celery或Zato,来简化异步任务的管理和调度。
  4. 服务类: 代码中使用了服务类(ScriptGenerator、VideoService、YoutubeService等)来执行业务逻辑。建议考虑使用工厂模式或依赖注入模式来创建和管理服务实例。
  5. 类型定义: 代码中使用了类型定义(GenerateScriptRequest、CropVideoRequest等)来定义API请求体。建议考虑使用 typing模块来定义更复杂的类型和结构。
  6. 实例变量: 代码中使用了实例变量(如task_id)来存储临时数据。建议考虑使用更安全和高效的方式,例如使用 fastapi.Request.state来存储请求状态数据。
  7. 重复代码: 代码中有重复的try-except块和异常处理代码。建议考虑抽取出公共的异常处理方法来减少重复代码。
  8. 测试: 代码中没有测试用例。建议考虑写入测试用例来确保API的正确性和可靠性。

以下是一些具体的代码优化建议:

  • generate_scriptcrop_video函数的try-except块可以合并为一个公共的异常处理函数。
  • start_subclip函数中可以使用fastapi.Request.state来存储任务ID和状态数据,而不是直接传递参数。
  • task_service.start_subclip函数可以考虑使用asyncio.create_task来异步执行任务,而不是直接使用BackgroundTasks。
  • VideoClipParams类型定义可以使用typing模块来定义更复杂的类型和结构。

Loading
Loading